zabetak commented on code in PR #6523:
URL: https://github.com/apache/hive/pull/6523#discussion_r3750550419
##########
iceberg/iceberg-handler/src/test/results/positive/llap/iceberg_bucket_map_join_1.q.out:
##########
Review Comment:
I more or less understood the reason behind the changes in this .q.out file.
What I am not sure is if we are losing something in terms of test coverage due
to the changes in cardinalities.
```
-- Each query has 1 join that can be converted to bucket join.
-- One of the query receives the small table from Map vertex while the other
recives it from Reducer vertex.
-- 4 configs enfoce MapJoin to be converted to one of the following joins:
-- 1. BucketMapJoin, 2. MapJoin, 3. VectorBucketMapJoin, 4. VectorMapJoin
```
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/type/HiveFunctionHelper.java:
##########
@@ -336,6 +339,11 @@ public RexNode getExpression(String functionText,
FunctionInfo fi,
RexUtil.flatten(call.getOperands(), call.getOperator()));
}
+ // Avoid creating incorrect expressions like $1 < NULL or $1 = NULL or
NULL = NULL
+ // which may be problematic for Calcite later on
+ RexSimplify rexSimplify = new RexSimplify(rexBuilder,
RelOptPredicateList.EMPTY, RexUtil.EXECUTOR);
+ expr = rexSimplify.simplifyComparisonWithNull(expr, RexUnknownAs.UNKNOWN);
+
return expr;
Review Comment:
```suggestion
return rexSimplify.simplifyComparisonWithNull(expr,
RexUnknownAs.UNKNOWN);
```
Small nit. Only apply if we need to update the PR
##########
ql/src/test/results/clientpositive/llap/allcolref_in_udf.q.out:
##########
@@ -76,10 +76,10 @@ STAGE PLANS:
Map Operator Tree:
TableScan
alias: a
- filterExpr: (UDFToDouble(key) < 100.0D) (type: boolean)
+ filterExpr: ((UDFToDouble(key) < 100.0D) and
(UDFToDouble(key) + 1.0D) is not null) (type: boolean)
Statistics: Num rows: 500 Data size: 89000 Basic stats:
COMPLETE Column stats: COMPLETE
Filter Operator
- predicate: (UDFToDouble(key) < 100.0D) (type: boolean)
+ predicate: ((UDFToDouble(key) < 100.0D) and
(UDFToDouble(key) + 1.0D) is not null) (type: boolean)
Review Comment:
It seems that this change falls under "3 ) Some IS [NOT] NULL
simplifications have been removed" category. I understand why it occurs but it
feels slightly as a regression or missed simplification. The additional `IS NOT
NULL` predicate does not seem to affect the result of the expression.
It's not blocking for the upgrade but wondering if it deserves a follow-up
ticket in order to restore the simplification (if its possible).
##########
ql/src/test/results/clientpositive/llap/dynamic_partition_pruning.q.out:
##########
@@ -581,10 +581,10 @@ STAGE PLANS:
Map Operator Tree:
TableScan
alias: srcpart
- filterExpr: CAST( ds AS DATE) is not null (type: boolean)
+ filterExpr: day(CAST( ds AS DATE)) is not null (type:
boolean)
Review Comment:
Is this a simplification regression? If we pass a DATE to the `day` function
can it change the nullability of the result?
##########
ql/src/test/results/clientpositive/llap/in_typecheck_char.q.out:
##########
@@ -390,7 +390,7 @@ POSTHOOK: Input: default@ax
CBO PLAN:
HiveProject(_c0=[$0])
HiveAggregate(group=[{}], agg#0=[count()])
- HiveFilter(condition=[IN($1, _UTF-16LE'a', _UTF-16LE'aa', _UTF-16LE'aaa',
_UTF-16LE'ab', _UTF-16LE'abc', _UTF-16LE'ac', _UTF-16LE'ba', _UTF-16LE'bb',
_UTF-16LE'bbb', _UTF-16LE'bc', _UTF-16LE'bca', _UTF-16LE'ca', _UTF-16LE'cab',
_UTF-16LE'cb', _UTF-16LE'cbc', _UTF-16LE'z')])
+ HiveFilter(condition=[IN($1, 'a ', 'aa ', 'aaa ', 'ab
', 'abc ', 'ac ', 'ba ', 'bb ', 'bbb ',
'bc ', 'bca ', 'ca ', 'cab ', 'cb ', 'cbc
', 'z ')])
Review Comment:
It's interesting that the literals in the physical plan below are already
padded. It means that correctness is probably not affected for Hive queries.
##########
ql/src/test/results/clientpositive/llap/external_jdbc_table2.q.out:
##########
@@ -716,7 +716,7 @@ STAGE PLANS:
hive.sql.query SELECT "ikey"
FROM (SELECT "ikey", "bkey", "dkey"
FROM "EXTERNAL_JDBC_SIMPLE_DERBY2_TABLE1") AS "t"
-WHERE ("bkey" = 10 AND "dkey" = 15.15 OR "bkey" = 20 AND "dkey" = 25.25) AND
"bkey" IN (10, 20) AND "dkey" IN (15.15, 25.25) AND "ikey" IS NOT NULL
+WHERE ("bkey" = 10 AND "dkey" = 1.515E1 OR "bkey" = 20 AND "dkey" = 2.525E1)
AND "bkey" IN (10, 20) AND "dkey" IN (1.515E1, 2.525E1) AND "ikey" IS NOT NULL
Review Comment:
It seems that the representation change of double and string literals does
not affect only the plans but also the actual SQL queries that we are pushing
down to various supported engines.
I guess we need to ensure that this change does not break the JDBC storage
handler so we may need a few tests for the supported DBMS (Postgres, MySQL,
MSSQL, Oracle). Since this test passes I guess that Derby coverage exists but
not sure if what holds for the rest. If there is documentation in the
respective databases that clearly states that this exponent literals are
supported then I don't insist in adding new test cases.
The proposed tests fit better in the Calcite repo rather than Hive so we
should log a CALCITE ticket if necessary.
##########
ql/src/test/results/clientpositive/llap/pointlookup6.q.out:
##########
@@ -99,6 +99,6 @@ POSTHOOK: Input: default@r_table
#### A masked pattern was here ####
CBO PLAN:
HiveProject(r_table.string_col=[$0])
- HiveFilter(condition=[OR(IS NULL(CAST($0):TIMESTAMP(9)),
IN(MINUTE(FLAG(MINUTE), CAST($0):TIMESTAMP(9)), 2, 10))])
+ HiveFilter(condition=[OR(IS NULL(MINUTE(FLAG(MINUTE),
CAST($0):TIMESTAMP(9))), IN(MINUTE(FLAG(MINUTE), CAST($0):TIMESTAMP(9)), 2,
10))])
Review Comment:
Can `MINUTE` affect the nullability of the result?
##########
ql/src/test/results/clientpositive/llap/input8.q.out:
##########
@@ -36,14 +36,14 @@ STAGE PLANS:
Map Operator Tree:
TableScan
alias: src1
- Statistics: Num rows: 25 Data size: 191 Basic stats:
COMPLETE Column stats: COMPLETE
+ Statistics: Num rows: 25 Data size: 2150 Basic stats:
COMPLETE Column stats: COMPLETE
Select Operator
- expressions: null (type: string), null (type: int), null
(type: double)
+ expressions: null (type: string),
UDFToInteger((UDFToDouble(key) - null)) (type: int), null (type: double)
Review Comment:
This may be a regression at least for Hive. The CAST in Hive does not throw
an error so subtracting null will always lead to null I guess.
##########
ql/src/test/results/clientpositive/llap/stats_histogram.q.out:
##########
@@ -710,16 +710,16 @@ STAGE PLANS:
Map Operator Tree:
TableScan
alias: test_stats
- filterExpr: (((d < 3.0) or (d > 7.0)) and (e > 0)) (type:
boolean)
+ filterExpr: ((e > 0) and ((d < 3.0) or (d > 7.0))) (type:
boolean)
Statistics: Num rows: 15 Data size: 1732 Basic stats:
COMPLETE Column stats: COMPLETE
Filter Operator
- predicate: (((d < 3.0) or (d > 7.0)) and (e > 0)) (type:
boolean)
- Statistics: Num rows: 8 Data size: 924 Basic stats:
COMPLETE Column stats: COMPLETE
+ predicate: ((e > 0) and ((d < 3.0) or (d > 7.0))) (type:
boolean)
+ Statistics: Num rows: 9 Data size: 1040 Basic stats:
COMPLETE Column stats: COMPLETE
Review Comment:
It's strange that predicate order affects the statistics. I guess it
shouldn't but this is minor and not directly relevant to the upgrade.
##########
ql/src/test/results/clientpositive/llap/vector_interval_mapjoin.q.out:
##########
@@ -206,26 +206,26 @@ STAGE PLANS:
Map Operator Tree:
TableScan
alias: vectortab_a_1korc
- filterExpr: (dt is not null and CAST( ts AS DATE) is not
null and s is not null) (type: boolean)
+ filterExpr: ((dt - CAST( ts AS DATE)) is not null and s is
not null) (type: boolean)
Review Comment:
Can the substraction change nullability?
##########
ql/src/main/resources/saffron.properties:
##########
@@ -0,0 +1,22 @@
+# -----------------------------------------------------------------------------
+# Calcite JVM-wide defaults for Hive.
+#
+# This file is read by org.apache.calcite.config.CalciteSystemProperty's
+# static initializer at class-load time. Setting properties here rather
+# than via System.setProperty() eliminates a potential class-load-order race.
+# Command-line JVM flags (-Dcalcite.foo=bar) still override this file;
+# see CalciteSystemProperty#loadProperties for the merge order.
+# -----------------------------------------------------------------------------
+
+# Prevent Calcite from normalizing RexNode digests. Hive relies on the
+# un-normalized form for plan output stability.
+calcite.enable.rexnode.digest.normalize = false
+
+# Default charset definition: matches Hive's HiveTypeFactory#getDefaultCharset.
+# Value is little-endian: this matches
+# org.apache.calcite.util.ConversionUtil#NATIVE_UTF16_CHARSET_NAME on
+# every architecture Hive is deployed on in practice (x86, x86_64,
+# aarch64 in default mode). On a hypothetical big-endian JVM, override with
+# -Dcalcite.default.charset=UTF-16BE -Dcalcite.default.nationalcharset=UTF-16BE
Review Comment:
Do we expect the users to change this property? Apart from query plan
display what's the impact of this property in Hive runtime? Do we expect query
results to change? Can it affect correctness?
The `ConversionUtil#NATIVE_UTF16_CHARSET_NAME` that is used extensively in
the code base is initialized automatically based on the endianess of the
system. Is there a risk in harcoding the default charset to little endian here?
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java:
##########
@@ -549,9 +550,8 @@ private ImmutableBitSet generateNewGroupset(Aggregate
aggregate, ImmutableBitSet
*/
private Aggregate rewriteGBConstantKeys(Aggregate aggregate, ImmutableBitSet
fieldsUsed,
ImmutableBitSet aggCallFields) {
- if ((aggregate.getIndicatorCount() > 0)
- || (aggregate.getGroupSet().isEmpty())
- || fieldsUsed.contains(aggregate.getGroupSet())) {
+ if (aggregate.getGroupSet().isEmpty()
+ || fieldsUsed.intersects(aggregate.getGroupSet())) {
Review Comment:
Is there a test case covering the added condition and the grouping sets?
##########
ql/pom.xml:
##########
@@ -365,6 +365,24 @@
<artifactId>hadoop-yarn-client</artifactId>
<optional>true</optional>
</dependency>
+ <!--
+ Runtime dependencies required by
org.apache.calcite.avatica:avatica-core:1.28.0
+ (pulled in by Calcite 1.42+): Avatica now uses Apache HttpComponents 5
for its
+ HTTP/JDBC transport, so hive-exec must bundle httpcore5/httpclient5 or
tests fail
+ with "NoClassDefFoundError: org/apache/hc/core5/util/Timeout". Avatica
declares
+ these as <scope>runtime</scope> transitively, but they must be declared
here
+ explicitly because Hive's enforcer bans org.apache.hc.core5/client5.**
imports
+ elsewhere and the hive-exec shade plugin only bundles ql-level
dependencies.
+ Can be removed once Hive migrates to HttpComponents 5 across the board.
+ -->
+ <dependency>
+ <groupId>org.apache.httpcomponents.core5</groupId>
+ <artifactId>httpcore5</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.httpcomponents.client5</groupId>
+ <artifactId>httpclient5</artifactId>
+ </dependency>
Review Comment:
Various other transitive dependencies from Calcite/Avatica are also added
explicitly to avoid classpath issues.
```
<dependency>
<groupId>org.codehaus.janino</groupId>
<artifactId>janino</artifactId>
</dependency>
<dependency>
<groupId>org.immutables</groupId>
<artifactId>value-annotations</artifactId>
</dependency>
```
I understand why we need to declare `httpcore5` here but I don't see the
connection with the enforcer checks. Can you please elaborate a bit more on
this?
The removal comment is also a bit confusing. I don't understand what needs
to be done after a global migration to httpcore5.
##########
ql/src/test/results/clientpositive/llap/auto_join13.q.out:
##########
@@ -87,7 +88,7 @@ STAGE PLANS:
sort order:
Statistics: Num rows: 1 Data size: 8 Basic
stats: COMPLETE Column stats: COMPLETE
value expressions: _col0 (type: bigint)
- Execution mode: vectorized, llap
+ Execution mode: llap
Review Comment:
The extra residual filter predicates seem to lead into loss of vectorization
so we may need to log a follow-up for the extra IS NOT NULL predicates.
##########
ql/src/test/results/clientpositive/llap/dynamic_partition_pruning.q.out:
##########
@@ -1885,37 +1885,40 @@ STAGE PLANS:
Map Operator Tree:
TableScan
alias: srcpart_double_hour_n0
- filterExpr: ((UDFToDouble(hour) = 11.0D) and
UDFToDouble(UDFToInteger((hr / 2.0D))) is not null) (type: boolean)
+ filterExpr: ((UDFToDouble(hour) = 11.0D) and hr is not null)
(type: boolean)
Statistics: Num rows: 2 Data size: 188 Basic stats: COMPLETE
Column stats: COMPLETE
Filter Operator
- predicate: ((UDFToDouble(hour) = 11.0D) and
UDFToDouble(UDFToInteger((hr / 2.0D))) is not null) (type: boolean)
+ predicate: ((UDFToDouble(hour) = 11.0D) and hr is not
null) (type: boolean)
Statistics: Num rows: 1 Data size: 94 Basic stats:
COMPLETE Column stats: COMPLETE
Select Operator
expressions: UDFToDouble(UDFToInteger((hr / 2.0D)))
(type: double)
outputColumnNames: _col0
Statistics: Num rows: 1 Data size: 8 Basic stats:
COMPLETE Column stats: COMPLETE
- Reduce Output Operator
- key expressions: _col0 (type: double)
- null sort order: z
- sort order: +
- Map-reduce partition columns: _col0 (type: double)
+ Filter Operator
+ predicate: _col0 is not null (type: boolean)
Review Comment:
This is the old predicate disguised as a new Filter operator.
##########
ql/src/test/results/clientpositive/llap/dynamic_partition_pruning.q.out:
##########
@@ -2301,10 +2307,10 @@ STAGE PLANS:
Map Operator Tree:
TableScan
alias: srcpart
- filterExpr: UDFToDouble(hr) is not null (type: boolean)
+ filterExpr: (UDFToDouble(hr) * 2.0D) is not null (type:
boolean)
Review Comment:
Is this a simplification regression? Can the multiplication with a constant
change the nullability of the result?
##########
ql/src/test/results/clientpositive/llap/dynamic_partition_pruning.q.out:
##########
@@ -1885,37 +1885,40 @@ STAGE PLANS:
Map Operator Tree:
TableScan
alias: srcpart_double_hour_n0
- filterExpr: ((UDFToDouble(hour) = 11.0D) and
UDFToDouble(UDFToInteger((hr / 2.0D))) is not null) (type: boolean)
+ filterExpr: ((UDFToDouble(hour) = 11.0D) and hr is not null)
(type: boolean)
Review Comment:
This simplification is called out in the description of the PR: "1 ) New IS
NOT NULL simplifications involving CAST, like in: dynamic_partition_pruning.q"
However, as indicated elsewhere the predicate is not going away but rather
moves a bit higher up disguised as a new `Filter` operator.
##########
ql/src/test/results/clientpositive/llap/auto_join2.q.out:
##########
@@ -118,19 +121,22 @@ STAGE PLANS:
Statistics: Num rows: 500 Data size: 49500 Basic
stats: COMPLETE Column stats: COMPLETE
value expressions: _col0 (type: string)
Filter Operator
- predicate: (key is not null and UDFToDouble(key) is not
null) (type: boolean)
+ predicate: key is not null (type: boolean)
Review Comment:
Instead of one `Filter` operator with a single conjunction we now have two
`Filter` operators with the conjunction split among them. In terms of
correctness and performance it shouldn't matter much but it is a bit strange
that this pattern appears despite the fact that we are using the
`FilterMergeRule`.
##########
ql/src/test/results/clientpositive/llap/explainuser_1.q.out:
##########
@@ -6562,7 +6562,7 @@ Stage-0
<-Select Operator [SEL_5] (rows=500 width=12)
Output:["_col0","_col1"]
Filter Operator [FIL_19] (rows=500 width=87)
- predicate:UDFToDouble(key) is not null
+ predicate:(UDFToDouble(key) + 1.0D) is not null
Review Comment:
(Not sure if already left the comment elsewhere)
Can the addition of a constant to a DOUBLE change the nullability of the
result? I guess it can lead to +Infinity but I am not sure if the latter is
considered null. I guess we don't have tests around this IS [NOT] NULL with
DOUBLE overflows/underflows so we may need to log a follow-up ticket to
establish the expected behavior in Hive cause this change may have correctness
implications.
##########
ql/src/test/results/clientpositive/llap/interval_3.q.out:
##########
@@ -159,10 +159,10 @@ STAGE PLANS:
Map Operator Tree:
TableScan
alias: d1
- filterExpr: CAST( d_date AS DATE) is not null (type: boolean)
+ filterExpr: (((((((CAST( d_date AS DATE) + INTERVAL'1-0') +
INTERVAL'0-2') + INTERVAL'5 00:00:00.000000000') + INTERVAL'0
04:00:00.000000000') + INTERVAL'0 00:10:00.000000000') + INTERVAL'0
00:00:09.000000000') is not null and (CAST( d_date AS DATE) + INTERVAL'1-2') is
not null) (type: boolean)
Statistics: Num rows: 1 Data size: 188 Basic stats: COMPLETE
Column stats: NONE
Filter Operator
- predicate: CAST( d_date AS DATE) is not null (type:
boolean)
+ predicate: (((((((CAST( d_date AS DATE) + INTERVAL'1-0') +
INTERVAL'0-2') + INTERVAL'5 00:00:00.000000000') + INTERVAL'0
04:00:00.000000000') + INTERVAL'0 00:10:00.000000000') + INTERVAL'0
00:00:09.000000000') is not null and (CAST( d_date AS DATE) + INTERVAL'1-2') is
not null) (type: boolean)
Review Comment:
The loss of simplifications around IS NOT NULL seems to have a big impact in
the Hive plans. We have to check if more conservative behavior makes sense for
Hive.
##########
ql/src/test/results/clientpositive/llap/external_jdbc_table_perf.q.out:
##########
@@ -3212,7 +3212,7 @@ WHERE "d_year" = 2000 AND "d_moy" = 1 AND "d_date_sk" IS
NOT NULL) AS "t10" ON "
hive.sql.query SELECT "i_item_id"
FROM (SELECT "i_item_id", "i_color"
FROM "ITEM") AS "t"
-WHERE "i_color" IN ('chiffon', 'lace', 'orchid') AND "i_item_id" IS NOT NULL
Review Comment:
This is a correctness fix right? It means that previously Hive was returning
wrong results. This test does not involve data otherwise we would see change in
the returned rows.
##########
ql/src/test/results/clientpositive/llap/materialized_view_rewrite_4.q.out:
##########
Review Comment:
Based on the git history the MV rewrite for the following query in
materialized_view_rewrite_4.q was working fine when the test was introduced.
```sql
select emps.deptno, count(distinct salary) as s
from emps
join dependents on (emps.empid = dependents.empid)
group by dependents.empid, emps.deptno
```
It seems that the rewritting was "broken" during
[HIVE-18839](https://issues.apache.org/jira/browse/HIVE-18839?focusedCommentId=16426416&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16426416)
and HIVE-19114 was raised as a follow-up.
The calcite upgrade restored the rewritting so we should also mark
HIVE-19114 as resolved when the PR is merged.
##########
ql/src/test/results/clientpositive/llap/udf_between.q.out:
##########
@@ -84,9 +84,9 @@ STAGE PLANS:
Processor Tree:
TableScan
alias: src
- filterExpr: (UDFToDouble(key) + 100.0D) NOT BETWEEN 100.0D AND
200.0D (type: boolean)
Review Comment:
It is interesting that we had NOT BETWEEN in the plan. I was under the
impression that this does not appear in the plan at all. Not super urgent but
would be curious to understand how this was introduced since it is relevant on
other improvements around BETWEEN that are currently under discussion in other
JIRA tickets.
##########
ql/src/test/results/clientpositive/perf/tpcds30tb/json/query30.q.out:
##########
@@ -1947,7 +1947,7 @@
"name": null
}
],
- "rowCount": 36000000
Review Comment:
The rowCount estimation has increased by many orders of magnitude. Are these
changes due to CALCITE-7083? It the new estimation better or worse?
##########
ql/src/test/results/clientpositive/perf/tpcds30tb/json/query38.q.out:
##########
@@ -1194,7 +1194,7 @@
"name": null
}
],
- "rowCount": 80000000
Review Comment:
The huge increase in rowCount estimation is somewhat worrisome. I am afraid
when these numbers are combined with other computations we may lead to inf
values.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
