[HUDI-3739] Fix handling of the isNotNull predicate in Data Skipping
#5224
Conversation
alexeykudinkin
changed the title
isNotNull predicate in Data SkippingisNotNull predicate in Data Skipping
| private val COLUMN_STATS_INDEX_MIN_VALUE_STAT_NAME = "minValue" | ||
| private val COLUMN_STATS_INDEX_MAX_VALUE_STAT_NAME = "maxValue" | ||
| private val COLUMN_STATS_INDEX_NUM_NULLS_STAT_NAME = "num_nulls" | ||
| private val COLUMN_STATS_INDEX_NULL_COUNT_STAT_NAME = "nullCount" |
There was a problem hiding this comment.
Can we not reuse HoodieMetadataPayload.* constants here as well?
There was a problem hiding this comment.
Good call. This was copied over from a legacy component w/o much afterthought. Addressed
| case IsNotNull(attribute: AttributeReference) => | ||
| getTargetIndexedColumnName(attribute, indexSchema) | ||
| .map(colName => EqualTo(genColNumNullsExpr(colName), Literal(0))) | ||
| .map(colName => LessThan(genColNumNullsExpr(colName), genColValueCountExpr)) |
There was a problem hiding this comment.
Filter colA is not null is the complement to colA is null then why the two have different translation (one has to depend on the valueCount while the other depends on Literal(0))?
I mean if colA is null is translated to GreaterThan(genColNumNullsExpr(colName), Literal(0)), then shouldn't colA is not null be translated to LessThanOrEqual(genColNumNullsExpr(colName), Literal(0))?
Or if you say that colA is not null should be translated to LessThan(genColNumNullsExpr(colName), genColValueCountExpr), then shouldn't colA is null be translated to GreaterThanOrEqual(genColNumNullsExpr(colName), genColValueCountExpr)?
There was a problem hiding this comment.
Ha! Great question.
The reason is logical fallacy: "colA is not null" != "there is no null in colA", instead it's "colA contains non-null" (you can check out some other expressions, there are many expressions in this list that carry such properties):
- "is null" means that the column has to contain null values (ie
nullCount> 0) - "is not null" means that the column has to contain non-null values (ie
nullCount<valueCount)
codope
added
priority:blocker
alexeykudinkin
force-pushed
the
ak/dskp-fix
branch
2 times, most recently
from
f054124 to
6cabaa7
Compare
codope
left a comment
codope
left a comment
There was a problem hiding this comment.
LGTM. Can land once the CI is green.
Updated fixtures;
alexeykudinkin
force-pushed
the
ak/dskp-fix
branch
from
6cabaa7 to
4075e19
Compare
alexeykudinkin
changed the title
isNotNull predicate in Data SkippingisNotNull predicate in Data Skipping
#5224) - Fix handling of the isNotNull predicate in Data Skipping
4 participants
Tips
What is the purpose of the pull request
Fix handling of the
isNotNullpredicate in Data SkippingBrief change log
isNotNullpredicate in Data SkippingVerify this pull request
This change added tests and can be verified as follows:
Committer checklist
Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.