wgtmac commented on code in PR #2082:
URL: https://github.com/apache/orc/pull/2082#discussion_r1880295428
##########
c++/src/sargs/PredicateLeaf.cc:
##########
@@ -390,10 +390,15 @@ namespace orc {
DIAGNOSTIC_POP
+ static bool colHasNullForwardCompatible(const proto::ColumnStatistics&
stats) {
Review Comment:
```suggestion
static bool mayHaveNull(const proto::ColumnStatistics& stats) {
```
##########
c++/src/sargs/PredicateLeaf.cc:
##########
@@ -390,10 +390,15 @@ namespace orc {
DIAGNOSTIC_POP
+ static bool colHasNullForwardCompatible(const proto::ColumnStatistics&
stats) {
Review Comment:
Why calling it `forward compatible`? It seems to be a bug from a legacy
writer IMHO.
##########
c++/test/TestPredicateLeaf.cc:
##########
@@ -672,7 +672,7 @@ namespace orc {
TEST(TestPredicateLeaf, testLackOfSataistics) {
PredicateLeaf pred(PredicateLeaf::Operator::IS_NULL,
PredicateDataType::STRING, 1, {});
EXPECT_EQ(TruthValue::YES_NO, evaluate(pred, createStringStats("c", "d",
true)));
- EXPECT_EQ(TruthValue::YES_NO_NULL, evaluate(pred,
createIncompleteNullStats()));
+ EXPECT_EQ(TruthValue::YES, evaluate(pred, createIncompleteNullStats()));
Review Comment:
IIRC, this is wrong because `not(is_null(x))` returns `TruthValue::NO_NULL`
after this change. For malformed stats, we should return `YES_NO_NULL` since we
cannot do anything.
##########
c++/src/sargs/PredicateLeaf.cc:
##########
@@ -701,24 +707,22 @@ namespace orc {
}
}
- // files written by trino may lack of hasnull field.
- if (!colStats.has_has_null()) return TruthValue::YES_NO_NULL;
Review Comment:
Is it better to keep this line so lines below do not need to change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
