zabetak commented on code in PR #6523:
URL: https://github.com/apache/hive/pull/6523#discussion_r3821817269
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/views/HiveAugmentSnapshotMaterializationRule.java:
##########
@@ -146,6 +146,12 @@ public void onMatch(RelOptRuleCall call) {
final RelBuilder relBuilder = call.builder();
relBuilder.push(tableScan);
+ if (snapshotId == null) {
+ // Avoid creating an incorrect expression $snapshotIdInputRef <= NULL
+ // which may be problematic for Calcite later on; instead use a special
value -1,
+ // which will be later interpreted by HivePushdownSnapshotFilterRule
(and removed)
+ snapshotId = -1L;
+ }
Review Comment:
@thomasrebele If you are satisfied with Ruben's answer please mark this
comment as resolved.
##########
iceberg/iceberg-handler/src/test/results/positive/dynamic_partition_pruning.q.out:
##########
@@ -1458,37 +1458,40 @@ STAGE PLANS:
Map Operator Tree:
TableScan
alias: srcpart_double_hour_n0
- filterExpr: ((UDFToDouble(hour) = 11.0D) and CAST(
UDFToInteger((hr / 2.0D)) AS STRING) 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 CAST(
UDFToInteger((hr / 2.0D)) AS STRING) is not null) (type: boolean)
+ predicate: ((UDFToDouble(hour) = 11.0D) and hr is not
null) (type: boolean)
Review Comment:
Will this be restored with CALCITE-7722?
##########
ql/pom.xml:
##########
@@ -365,6 +365,14 @@
<artifactId>hadoop-yarn-client</artifactId>
<optional>true</optional>
</dependency>
+ <dependency>
+ <groupId>org.apache.httpcomponents.core5</groupId>
+ <artifactId>httpcore5</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.httpcomponents.client5</groupId>
+ <artifactId>httpclient5</artifactId>
+ </dependency>
Review Comment:
@thomasrebele If you are happy with Ruben's answer please mark the
discussion as resolved.
##########
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:
1. How can we ensure that we won't forget to do the necessary changes in
Hive with the next calcite upgrade?
a. Should we add the override now?
b. Should we create a JIRA and link it to the next upgrade ticket?
c. Should we add an entry org.apache.hadoop.hive.ql.optimizer.calcite.Bug
and reference it from `HiveExtractDate`?
2. Should we modify the safety guarantees of EXTRACT in Calcite? If yes, how:
a. Override org.apache.calcite.sql.fun.SqlExtractFunction#isSafeOperator
b. Add SqlKind.EXTRACT in
org.apache.calcite.rex.RexSimplify.SafeRexVisitor#SafeRexVisitor?
##########
iceberg/iceberg-handler/src/test/results/positive/llap/iceberg_bucket_map_join_1.q.out:
##########
Review Comment:
Great, the new plans looks fine!
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveTypeSystemImpl.java:
##########
@@ -189,4 +181,39 @@ public RelDataType deriveSumType(RelDataTypeFactory
typeFactory,
return argumentType;
}
+ /**
+ * Overridden because CALCITE-6464 changed the default behavior to match
MS-SQL-Server-style algorithm,
+ * which can cause a drop in the scale computation, hence a precision loss
in certain cases.
+ * We override this method to keep the "old behavior" (pre-CALCITE-6464); an
alternative could be not
+ * overridding it (and keep the new Calcite default MS-SQL-style
decimal-divide semantics), but that
+ * would lead to "regressions" (precision loss) and would require test
adjustments.
+ */
+ @Override
+ public RelDataType deriveDecimalDivideType(RelDataTypeFactory typeFactory,
+ RelDataType type1, RelDataType type2) {
+ if (SqlTypeUtil.isExactNumeric(type1) && SqlTypeUtil.isExactNumeric(type2)
&&
+ (SqlTypeUtil.isDecimal(type1) || SqlTypeUtil.isDecimal(type2))) {
+ // Java numeric will always have invalid precision/scale,
+ // use its default decimal precision/scale instead.
+ type1 = RelDataTypeFactoryImpl.isJavaType(type1) ?
typeFactory.decimalOf(type1) : type1;
+ type2 = RelDataTypeFactoryImpl.isJavaType(type2) ?
typeFactory.decimalOf(type2) : type2;
+ int p1 = type1.getPrecision();
+ int p2 = type2.getPrecision();
+ int s1 = type1.getScale();
+ int s2 = type2.getScale();
+
+ final int maxNumericPrecision = getMaxNumericPrecision();
+ int dout = Math.min(p1 - s1 + s2, maxNumericPrecision);
+ int scale = Math.max(6, s1 + p2 + 1);
+ scale = Math.min(scale, maxNumericPrecision - dout);
+ scale = Math.min(scale, getMaxNumericScale());
+
+ int precision = dout + scale;
+ assert precision <= maxNumericPrecision;
+ assert precision > 0;
+ return typeFactory.createSqlType(SqlTypeName.DECIMAL, precision, scale);
+ }
+ return null;
+ }
+
Review Comment:
I see that multiple tests with DECIMAL types are affected. The impact is
visible in the plans but also on the query results.
Although the code is not identical the method closely resembles the logic in
`GenericUDFOPDivide#deriveResultDecimalTypeInfo`. The comments there indicate
that behavior in Hive is the same with MSSQL so it makes total sense to have
this method here.
A potential improvement would be to refactor the code here to use
`GenericUDFOPDivide` and avoid code duplication but its rather low priority so
we can leave things as is for now.
##########
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:
Discussed elsewhere so marking this as resolved.
##########
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:
Resolving this and we can continue on the other discussion.
##########
ql/src/test/results/clientpositive/perf/tpcds30tb/json/query30.q.out:
##########
@@ -1947,7 +1947,7 @@
"name": null
}
],
- "rowCount": 36000000
Review Comment:
Thanks for the explanation! In other words, we already deal with big row
counts just slightly deeper in the plan. My initial concern about manipulations
of big numbers ending up in infinite values goes away since we already had such
numbers before.
CALCITE-7083 seems reasonable so we can say that new estimations are indeed
an improvement that we can accept. Interestingly, the TPC-DS query plans didn't
change shape (join order is the same) so for this benchmark this change will
not have an impact on performance.
##########
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:
Thanks for the explanation. I added a comment on
[HIVE-29760](https://issues.apache.org/jira/browse/HIVE-29760?focusedCommentId=18105930&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-18105930)
to document that HivePointLookupOptimizerRule was generating a NOT BETWEEN
operator.
Based on the discussion here (and point 12 of the description) it seems the
HivePointLookupOptimizerRule has more redundancy than before. It may be
possible to drop the rule after the upgrade to 1.42.0. The proposed
cleanup/removal is already tracked under
https://issues.apache.org/jira/browse/HIVE-28907.
In terms of the upgrade here, there is nothing more to do so resolving the
comment.
##########
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:
Since this simplification is only possible with Hive semantics, we would
have to log a HIVE follow-up ticket to restore this. Not sure yet if we need to
globally switch to SAFE_CAST or if there is another tweak possible but we can
investigate the different alternatives once we wrap-up the upgrade.
##########
ql/src/test/results/clientpositive/perf/tpcds30tb/json/query38.q.out:
##########
@@ -1194,7 +1194,7 @@
"name": null
}
],
- "rowCount": 80000000
Review Comment:
Resolving this comment. Let's continue under the referenced dicsussion.
##########
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:
Is this going to be restored by CALCITE-7722?
##########
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:
nit: If its just about plan display/verbosity then we could just document
this aspect and remove the whole discussion about big/little-endian since its
gonna be rare anyways. We could remove also the override directives from here
since there are relevant instructions at the top of the file.
(Not worth updating the PR just for this)
--
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]
