chore: Remove redundant parquet.enable.dictionary ConfigMatrix from SQL tests#3866
Open
andygrove wants to merge 1 commit intoapache:mainfrom
Open
chore: Remove redundant parquet.enable.dictionary ConfigMatrix from SQL tests#3866andygrove wants to merge 1 commit intoapache:mainfrom
parquet.enable.dictionary ConfigMatrix from SQL tests#3866andygrove wants to merge 1 commit intoapache:mainfrom
Conversation
…QL tests The ConfigMatrix directive for parquet.enable.dictionary was added as boilerplate to all SQL test files but is redundant since the tests generally do not create tables with duplicate string data that would be dictionary-encoded. Also update contributor guides to stop recommending this directive for new tests.
andygrove
changed the title
parquet.enable.dictionary ConfigMatrix from SQL tests
Contributor
|
Not against for this change, but do we plan to add dictionary encoded parquet tests for the sql tests in the future? |
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?
N/A - minor cleanup
Rationale for this change
The
-- ConfigMatrix: parquet.enable.dictionary=false,truedirective was added as boilerplate to all 136 SQL expression test files. This causes each test to run twice (once with dictionary encoding enabled, once disabled), but is redundant because the tests generally do not create tables with duplicate string data that would actually be dictionary-encoded by Parquet. Removing it halves the number of SQL file test cases without reducing coverage.What changes are included in this PR?
-- ConfigMatrix: parquet.enable.dictionary=false,truedirective from all 136 SQL test filessql-file-tests.md) to stop recommending this directive for new tests and use a more meaningful ConfigMatrix exampleadding_a_new_expression.md) to remove the directive from the example SQL test fileHow are these changes tested?
The SQL file tests still run, just without the redundant dictionary encoding matrix. Existing test coverage is preserved since the dictionary encoding setting has no meaningful effect on these tests.