Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CALCITE-6400] MAP_ENTRIES function should throw if a key value is null #3787

Merged
merged 1 commit into from May 20, 2024
Merged

[CALCITE-6400] MAP_ENTRIES function should throw if a key value is null #3787

merged 1 commit into from May 20, 2024

Conversation

caicancai
Copy link
Member

@caicancai caicancai commented May 4, 2024 •
edited

@caicancai caicancai changed the title [CALCITE-6400] MAP_ENTRIES is not allowed to be empty [CALCITE-6400] MAP_ENTRIES simply does not allow the key to be empty May 4, 2024
@caicancai caicancai changed the title [CALCITE-6400] MAP_ENTRIES simply does not allow the key to be empty [CALCITE-6400] MAP_ENTRIES does not allow null as a map key value May 5, 2024
@caicancai
Copy link
Member Author

In spark, this is an error that occurs in function execution, not in type verification, so I modified the method in sqlfunctions

@chucheng92
Copy link
Member

looks good to me in general. just one question, does this exception have to be thrown at runtime? i noticed that the exception is thrown directly in SqlFunctions.

@caicancai
Copy link
Member Author

caicancai commented May 7, 2024 •
edited

looks good to me in general. just one question, does this exception have to be thrown at runtime? i noticed that the exception is thrown directly in SqlFunctions.

@chucheng92 Thank you for your review. I write this because spark has this mechanism, but calcite is usually checked in OperadandChecker.
@mihaibudiu Which method do you think should be used?

@caicancai caicancai requested a review from mihaibudiu May 7, 2024 14:50
@mihaibudiu
Copy link
Contributor

I don't know what "opera" and "checker" are.

@caicancai caicancai requested a review from chucheng92 May 8, 2024 01:51
@caicancai
Copy link
Member Author

I don't know what "opera" and "checker" are.

Sorry, typo, it should be OperaandChecker

@chucheng92
Copy link
Member

chucheng92 commented May 8, 2024 •
edited

@caicancai could you improve the current commit name? It is different from jira and pr names.

@chucheng92 chucheng92 changed the title [CALCITE-6400] MAP_ENTRIES does not allow null as a map key value [CALCITE-6400] MAP_ENTRIES does not allow null as a map key May 8, 2024
@caicancai caicancai force-pushed the 6400 branch 2 times, most recently from ec90eb8 to 4cec326 Compare May 8, 2024 13:59
@caicancai caicancai changed the title [CALCITE-6400] MAP_ENTRIES does not allow null as a map key [CALCITE-6400] MAP_ENTRIES allow null as a map key May 9, 2024
@caicancai caicancai changed the title [CALCITE-6400] MAP_ENTRIES allow null as a map key [CALCITE-6400] MAP_ENTRIES allows null as a map key May 11, 2024
@mihaibudiu
Copy link
Contributor

The issue is about map_entries, but this PR fixes the arguments to the map constructor.

@caicancai
Copy link
Member Author

The issue is about map_entries, but this PR fixes the arguments to the map constructor.

Thanks, I will modify the jira summary, the description is being discussed in jira

@caicancai caicancai changed the title [CALCITE-6400] MAP_ENTRIES allows null as a map key [CALCITE-6400] MAP_ENTRIES function should throw if a key value is null May 14, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 14, 2024

Quality Gate Passed Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@macroguo-ghy macroguo-ghy merged commit 43c8848 into apache:main May 20, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chucheng92 chucheng92 chucheng92 approved these changes

@mihaibudiu mihaibudiu mihaibudiu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@caicancai @chucheng92 @mihaibudiu @macroguo-ghy