Repository navigation
Refuse add_files partition inference for a file holding a value and NULL - #4264
Open
Rahul-cs50 wants to merge 1 commit into
Open
Rahul-cs50 wants to merge 1 commit into
Rahul-cs50 wants to merge 1 commit into
Conversation
A file holding a value and NULL in a partition source column has equal min and max, so add_files registered it under that value's partition and IS NULL filters pruned it. Refuse it like a file holding two values. Closes apache#4191
This branch has not been deployed
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
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.
Closes #4191
Rationale for this change
add_filesinfers a file's partition from the parquet min and max of the source column. That ignores the null count. A file holding'a'and NULL has min = max ='a', so it is registered under partition'a'.s IS NULLthen prunes it, and the scan in the issue returns[]instead of[2].A value and NULL are two partition values, just as
'a'and'b'are.add_filesalready refuses a file holding'a'and'b'. This change refuses a file holding a value and NULL in the same way, using the null count that is already inDataFileStatistics. A file whose column is all NULL still goes to the NULL partition, as before.This PR covers only the value-and-NULL case. The issue mentions two other cases, and I left both alone:
nan_value_countsis not filled from parquet metadata, so this check cannot see NaN.write.metadata.metrics.default=none). The file gets aNonepartition, andmkdocs/docs/api.mddocuments that behavior. Changing it is a call for maintainers.Are these changes tested?
Yes.
test_add_files_refuses_partition_value_with_nullsintests/catalog/test_catalog_behaviors.pyruns on each catalog fixture:'a'file and an all-NULL file, and checks thats = 'a'ands IS NULLreturn the right rows.'a'and NULL is refused and nothing is added.The test fails on
main(no error is raised) and passes with this change. Unit tests (tests/without integration and cloud suites): 4495 passed; the 4 failures are Kerberos tests intests/catalog/test_hive.pythat fail the same way onmainin my environment.ruff checkandruff format --checkare clean.Are there any user-facing changes?
Yes.
add_filesnow raisesValueErrorfor a file where an identity-partition source column, or one with an order-preserving transform, holds both values and NULLs. Before, such a file was silently given the wrong partition.