You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "zabetak (via GitHub)" <gi...@apache.org> on 2023/03/21 15:59:46 UTC
[GitHub] [hive] zabetak opened a new pull request, #4135: HIVE-27157: AssertionError when inferring return type for unix_timestamp function
zabetak opened a new pull request, #4135:
URL: https://github.com/apache/hive/pull/4135
### What changes were proposed in this pull request?
Change the implementation of unix_timestamp operators to avoid the AssertionError and infer the return type correctly; always BIGINT.
Break the inheritance relation with SqlAbstractTimeFunction and change the SqlFunctionCategory from TIMEDATE to NUMERIC; unix_timestamp is not a time function since the result is never among DATE, TIME, or TIMESTAMP.
Change the operand type checker to a more truthful implementation; the type checker is not really used at the moment but it is better to have something realistic there instead of null or something completely wrong.
### Why are the changes needed?
Calls to `inferReturnType` method for `unix_timestamp` operators always lead to `AssertionError`.
Contrary to operand type checking and operand type inference that are not really relevant for Hive (the latter is not using the `SqlValidator` logic), the return type inference is important since it may kick in some calls to `RelBuilder/RexBuilder` APIs. Such calls exist in older versions of Hive and are widely used in Calcite's built-in rules.
### Does this PR introduce _any_ user-facing change?
In this version no, but in older versions of Hive it can fix queries failing with `AssertionError`.
### How was this patch tested?
```
mvn test -Dtest=TestSqlOperatorInferReturnType
```
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #4135: HIVE-27157: AssertionError when inferring return type for unix_timestamp function
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4135:
URL: https://github.com/apache/hive/pull/4135#issuecomment-1479395909
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4135)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4135&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4135&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4135&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4135&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4135&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4135&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4135&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4135&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4135&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4135&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4135&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4135&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4135&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4135&metric=duplicated_lines_density&view=list) No Duplication information
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak closed pull request #4135: HIVE-27157: AssertionError when inferring return type for unix_timestamp function
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak closed pull request #4135: HIVE-27157: AssertionError when inferring return type for unix_timestamp function
URL: https://github.com/apache/hive/pull/4135
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kasakrisz commented on a diff in pull request #4135: HIVE-27157: AssertionError when inferring return type for unix_timestamp function
Posted by "kasakrisz (via GitHub)" <gi...@apache.org>.
kasakrisz commented on code in PR #4135:
URL: https://github.com/apache/hive/pull/4135#discussion_r1145879714
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveToUnixTimestampSqlOperator.java:
##########
@@ -18,15 +18,24 @@
package org.apache.hadoop.hive.ql.optimizer.calcite.reloperators;
-import org.apache.calcite.sql.fun.SqlAbstractTimeFunction;
-import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.SqlFunction;
+import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.ReturnTypes;
/**
* Sql UNIX_TIMESTAMP calcite operator.
*/
-public class HiveToUnixTimestampSqlOperator extends SqlAbstractTimeFunction {
- public static final HiveToUnixTimestampSqlOperator INSTANCE = new HiveToUnixTimestampSqlOperator();
- protected HiveToUnixTimestampSqlOperator() {
- super("UNIX_TIMESTAMP", SqlTypeName.BIGINT);
- }
+public class HiveToUnixTimestampSqlOperator {
+ public static final SqlFunction INSTANCE =
+ new SqlFunction("UNIX_TIMESTAMP", SqlKind.OTHER_FUNCTION, ReturnTypes.BIGINT, null,
+ OperandTypes.or(OperandTypes.NILADIC,
+ OperandTypes.or(OperandTypes.STRING, OperandTypes.TIMESTAMP, OperandTypes.DATE),
+ OperandTypes.STRING_STRING), SqlFunctionCategory.NUMERIC) {
+ @Override
+ public boolean isDynamicFunction() {
+ return true;
+ }
Review Comment:
Could you please share a link to those code places?`SqlFunction` seems to be immutable.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #4135: HIVE-27157: AssertionError when inferring return type for unix_timestamp function
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4135:
URL: https://github.com/apache/hive/pull/4135#issuecomment-1478373963
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4135)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4135&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4135&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4135&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4135&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4135&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4135&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4135&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4135&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4135&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4135&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4135&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4135&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4135&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4135&metric=duplicated_lines_density&view=list) No Duplication information
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a diff in pull request #4135: HIVE-27157: AssertionError when inferring return type for unix_timestamp function
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #4135:
URL: https://github.com/apache/hive/pull/4135#discussion_r1145980904
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveToUnixTimestampSqlOperator.java:
##########
@@ -18,15 +18,24 @@
package org.apache.hadoop.hive.ql.optimizer.calcite.reloperators;
-import org.apache.calcite.sql.fun.SqlAbstractTimeFunction;
-import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.SqlFunction;
+import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.ReturnTypes;
/**
* Sql UNIX_TIMESTAMP calcite operator.
*/
-public class HiveToUnixTimestampSqlOperator extends SqlAbstractTimeFunction {
- public static final HiveToUnixTimestampSqlOperator INSTANCE = new HiveToUnixTimestampSqlOperator();
- protected HiveToUnixTimestampSqlOperator() {
- super("UNIX_TIMESTAMP", SqlTypeName.BIGINT);
- }
+public class HiveToUnixTimestampSqlOperator {
+ public static final SqlFunction INSTANCE =
+ new SqlFunction("UNIX_TIMESTAMP", SqlKind.OTHER_FUNCTION, ReturnTypes.BIGINT, null,
+ OperandTypes.or(OperandTypes.NILADIC,
+ OperandTypes.or(OperandTypes.STRING, OperandTypes.TIMESTAMP, OperandTypes.DATE),
+ OperandTypes.STRING_STRING), SqlFunctionCategory.NUMERIC) {
+ @Override
+ public boolean isDynamicFunction() {
+ return true;
+ }
Review Comment:
Here is one place where we convert from one operator to the other:
https://github.com/apache/hive/blob/e401b5b3c6ac7064615dc9f28cc9d974b815b070/ql/src/java/org/apache/hadoop/hive/ql/parse/type/HiveFunctionHelper.java#L317
Here is another place that only kicks in for one of the two operators:
https://github.com/apache/hive/blob/e401b5b3c6ac7064615dc9f28cc9d974b815b070/ql/src/java/org/apache/hadoop/hive/ql/parse/DruidSqlOperatorConverter.java#L99
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kasakrisz commented on a diff in pull request #4135: HIVE-27157: AssertionError when inferring return type for unix_timestamp function
Posted by "kasakrisz (via GitHub)" <gi...@apache.org>.
kasakrisz commented on code in PR #4135:
URL: https://github.com/apache/hive/pull/4135#discussion_r1145116056
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveToUnixTimestampSqlOperator.java:
##########
@@ -18,15 +18,24 @@
package org.apache.hadoop.hive.ql.optimizer.calcite.reloperators;
-import org.apache.calcite.sql.fun.SqlAbstractTimeFunction;
-import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.SqlFunction;
+import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.ReturnTypes;
/**
* Sql UNIX_TIMESTAMP calcite operator.
*/
-public class HiveToUnixTimestampSqlOperator extends SqlAbstractTimeFunction {
- public static final HiveToUnixTimestampSqlOperator INSTANCE = new HiveToUnixTimestampSqlOperator();
- protected HiveToUnixTimestampSqlOperator() {
- super("UNIX_TIMESTAMP", SqlTypeName.BIGINT);
- }
+public class HiveToUnixTimestampSqlOperator {
+ public static final SqlFunction INSTANCE =
+ new SqlFunction("UNIX_TIMESTAMP", SqlKind.OTHER_FUNCTION, ReturnTypes.BIGINT, null,
+ OperandTypes.or(OperandTypes.NILADIC,
+ OperandTypes.or(OperandTypes.STRING, OperandTypes.TIMESTAMP, OperandTypes.DATE),
+ OperandTypes.STRING_STRING), SqlFunctionCategory.NUMERIC) {
+ @Override
+ public boolean isDynamicFunction() {
+ return true;
+ }
Review Comment:
Is this the same constant definition in `HiveUnixTimestampSqlOperator`? Is it worth referencing it from both classes?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a diff in pull request #4135: HIVE-27157: AssertionError when inferring return type for unix_timestamp function
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #4135:
URL: https://github.com/apache/hive/pull/4135#discussion_r1145203540
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveToUnixTimestampSqlOperator.java:
##########
@@ -18,15 +18,24 @@
package org.apache.hadoop.hive.ql.optimizer.calcite.reloperators;
-import org.apache.calcite.sql.fun.SqlAbstractTimeFunction;
-import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.SqlFunction;
+import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.ReturnTypes;
/**
* Sql UNIX_TIMESTAMP calcite operator.
*/
-public class HiveToUnixTimestampSqlOperator extends SqlAbstractTimeFunction {
- public static final HiveToUnixTimestampSqlOperator INSTANCE = new HiveToUnixTimestampSqlOperator();
- protected HiveToUnixTimestampSqlOperator() {
- super("UNIX_TIMESTAMP", SqlTypeName.BIGINT);
- }
+public class HiveToUnixTimestampSqlOperator {
+ public static final SqlFunction INSTANCE =
+ new SqlFunction("UNIX_TIMESTAMP", SqlKind.OTHER_FUNCTION, ReturnTypes.BIGINT, null,
+ OperandTypes.or(OperandTypes.NILADIC,
+ OperandTypes.or(OperandTypes.STRING, OperandTypes.TIMESTAMP, OperandTypes.DATE),
+ OperandTypes.STRING_STRING), SqlFunctionCategory.NUMERIC) {
+ @Override
+ public boolean isDynamicFunction() {
+ return true;
+ }
Review Comment:
It was the same before and it remains the same now. Ultimately, I would like to get rid one of them completely but it was not trivial to do now (correlation with code in Druid storage handler) thus in order to keep the PR minimal I decided to defer this to HIVE-27162.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a diff in pull request #4135: HIVE-27157: AssertionError when inferring return type for unix_timestamp function
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #4135:
URL: https://github.com/apache/hive/pull/4135#discussion_r1145205314
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveToUnixTimestampSqlOperator.java:
##########
@@ -18,15 +18,24 @@
package org.apache.hadoop.hive.ql.optimizer.calcite.reloperators;
-import org.apache.calcite.sql.fun.SqlAbstractTimeFunction;
-import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.SqlFunction;
+import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.ReturnTypes;
/**
* Sql UNIX_TIMESTAMP calcite operator.
*/
-public class HiveToUnixTimestampSqlOperator extends SqlAbstractTimeFunction {
- public static final HiveToUnixTimestampSqlOperator INSTANCE = new HiveToUnixTimestampSqlOperator();
- protected HiveToUnixTimestampSqlOperator() {
- super("UNIX_TIMESTAMP", SqlTypeName.BIGINT);
- }
+public class HiveToUnixTimestampSqlOperator {
+ public static final SqlFunction INSTANCE =
+ new SqlFunction("UNIX_TIMESTAMP", SqlKind.OTHER_FUNCTION, ReturnTypes.BIGINT, null,
+ OperandTypes.or(OperandTypes.NILADIC,
+ OperandTypes.or(OperandTypes.STRING, OperandTypes.TIMESTAMP, OperandTypes.DATE),
+ OperandTypes.STRING_STRING), SqlFunctionCategory.NUMERIC) {
+ @Override
+ public boolean isDynamicFunction() {
+ return true;
+ }
Review Comment:
Forgot to mention that there are some places in the code that rely on the fact that there are two different instances.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org