feat: Error when a SHOW command is passed in with an accompanying non-existant variable#11540
Merged
alamb merged 5 commits intoapache:mainfrom Jul 22, 2024
itsjunetime:main
Merged
feat: Error when a SHOW command is passed in with an accompanying non-existant variable#11540alamb merged 5 commits intoapache:mainfrom itsjunetime:main
alamb merged 5 commits intoapache:mainfrom
itsjunetime:main
Conversation
…test to see if that fixes CI
…iline error message/placement
alamb
approved these changes
Jul 19, 2024
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @itsjunetime, this looks like a very nice improvement to me
The CI failure looks unrelated to this PR and I have retriggered it
---- fuzz_cases::join_fuzz::test_anti_join_1k_filtered stdout ----
thread 'fuzz_cases::join_fuzz::test_anti_join_1k_filtered' panicked at datafusion/core/tests/fuzz_cases/join_fuzz.rs:537:17:
assertion `left == right` failed: HashJoinExec and SortMergeJoinExec produced different row counts, batch_size: 1
left: 970
right: 971
stack backtrace:
I also saw the same error on another PR so I will file a ticket: #11527 (comment)
Contributor
|
Filed #11555 |
Contributor
|
Thanks again @itsjunetime |
Lordworms
pushed a commit
to Lordworms/arrow-datafusion
that referenced
this pull request
Jul 23, 2024
…-existant variable (apache#11540) * feat: Error when a SHOW command is passed in with an accompanying non-existant variable * fix: Run fmt * Switch to 'query error' instead of 'statement error' in sqllogictest test to see if that fixes CI * Move some errors in sqllogictest to line above to maybe fix CI * Fix (hopefully final) failing information_schema slt test due to multiline error message/placement
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #11529
Rationale for this change
This is a very simple implementation for the change suggested in #11529, and is easy to review and comprehend.
What changes are included in this PR?
This causes datafusion to return an error when someone attempts to execute a command that contains a
SHOWstatement whose variable does not exist in theinformation_schema.df_settingstable. This provides a better UX for the user, as they get told exactly what's going on (with a clear error) when they execute this command, instead of getting an empty table as the result (which was happening previously).Are these changes tested?
Yes - Additions were made to the sqllogictest tests to cover the use case addressed here.
Are there any user-facing changes?
I think this counts as a user-facing change - it would be easy to rely on the behavior of "SHOW will return an empty result if doesn't exist" and so it's very possible someone does rely on that.
I'd be happy to update documentation if that's necessary with this change, though I'm somewhat uncertain as to whether this warrants that - the previous behavior wasn't specified in docs, so I don't know if it is necessary here. It probably couldn't hurt, I think.