You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2021/02/16 11:34:19 UTC
[GitHub] [hive] pgaref opened a new pull request #1982: HIVE-24778: Unify hive.strict.timestamp.conversion and hive.strict.checks.type.safety properties
pgaref opened a new pull request #1982:
URL: https://github.com/apache/hive/pull/1982
### What changes were proposed in this pull request?
The majority of strict type checks can be controlled by hive.strict.checks.type.safety property. HIVE-24157 introduced another property, namely hive.strict.timestamp.conversion, to control the implicit comparisons between numerics and timestamps.
The name and description of hive.strict.checks.type.safety imply that the property covers all strict checks so having others for specific cases appears confusing and can easily lead to unexpected behavior.
This PR unifies those properties to facilitate configuration and improve code reuse.
### Why are the changes needed?
Reduce conf complexity
### Does this PR introduce _any_ user-facing change?
Unifies hive.strict.timestamp.conversion and hive.strict.checks.type.safety properties
### How was this patch tested?
Existing tests
----------------------------------------------------------------
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.
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 change in pull request #1982: HIVE-24778: Unify hive.strict.timestamp.conversion and hive.strict.checks.type.safety properties
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #1982:
URL: https://github.com/apache/hive/pull/1982#discussion_r578001413
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/udf/TimestampCastRestrictorResolver.java
##########
@@ -45,7 +45,7 @@
public TimestampCastRestrictorResolver(UDFMethodResolver parentResolver) {
this.parentResolver = parentResolver;
SessionState ss = SessionState.get();
- if (ss != null && ss.getConf().getBoolVar(ConfVars.HIVE_STRICT_TIMESTAMP_CONVERSION)) {
+ if (ss != null && ss.getConf().getBoolVar(ConfVars.HIVE_STRICT_CHECKS_TYPE_SAFETY)) {
Review comment:
This means that for all the other kind of strict checks we are vulnerable to conversions happening during init right?
----------------------------------------------------------------
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.
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] pgaref commented on a change in pull request #1982: HIVE-24778: Unify hive.strict.timestamp.conversion and hive.strict.checks.type.safety properties
Posted by GitBox <gi...@apache.org>.
pgaref commented on a change in pull request #1982:
URL: https://github.com/apache/hive/pull/1982#discussion_r578334938
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/udf/TimestampCastRestrictorResolver.java
##########
@@ -45,7 +45,7 @@
public TimestampCastRestrictorResolver(UDFMethodResolver parentResolver) {
this.parentResolver = parentResolver;
SessionState ss = SessionState.get();
- if (ss != null && ss.getConf().getBoolVar(ConfVars.HIVE_STRICT_TIMESTAMP_CONVERSION)) {
+ if (ss != null && ss.getConf().getBoolVar(ConfVars.HIVE_STRICT_CHECKS_TYPE_SAFETY)) {
Review comment:
Hey Stamati, it seems that if if its not a direct conversion that can be checked with LOSSY_CONVERSIONS, it has to be part of the UDF init. For example:
- Bigint cannot be cast to a double: https://github.com/apache/hive/blob/adf426a87e60e0b99dec7e8aee7d1a5e12462161/ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeGenericFuncDesc.java#L248
- Date only allows timestamps and Strings https://github.com/apache/hive/blob/853e1d1f9d17ef61637f094b7bd2cccf44931124/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFToDate.java#L68
----------------------------------------------------------------
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.
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] github-actions[bot] commented on pull request #1982: HIVE-24778: Unify hive.strict.timestamp.conversion and hive.strict.checks.type.safety properties
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #1982:
URL: https://github.com/apache/hive/pull/1982#issuecomment-855489270
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.
--
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.
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 change in pull request #1982: HIVE-24778: Unify hive.strict.timestamp.conversion and hive.strict.checks.type.safety properties
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #1982:
URL: https://github.com/apache/hive/pull/1982#discussion_r586504261
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/udf/TimestampCastRestrictorResolver.java
##########
@@ -45,7 +45,7 @@
public TimestampCastRestrictorResolver(UDFMethodResolver parentResolver) {
this.parentResolver = parentResolver;
SessionState ss = SessionState.get();
- if (ss != null && ss.getConf().getBoolVar(ConfVars.HIVE_STRICT_TIMESTAMP_CONVERSION)) {
+ if (ss != null && ss.getConf().getBoolVar(ConfVars.HIVE_STRICT_CHECKS_TYPE_SAFETY)) {
Review comment:
Putting a note here so that we don't forget. The SQL standard forbids all kind of explicit casts from DATE, TIME, DATETIME to non char datatypes and does not mention anything about implicit casts.
----------------------------------------------------------------
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.
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] pgaref commented on a change in pull request #1982: HIVE-24778: Unify hive.strict.timestamp.conversion and hive.strict.checks.type.safety properties
Posted by GitBox <gi...@apache.org>.
pgaref commented on a change in pull request #1982:
URL: https://github.com/apache/hive/pull/1982#discussion_r577705382
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/udf/TimestampCastRestrictorResolver.java
##########
@@ -45,7 +45,7 @@
public TimestampCastRestrictorResolver(UDFMethodResolver parentResolver) {
this.parentResolver = parentResolver;
SessionState ss = SessionState.get();
- if (ss != null && ss.getConf().getBoolVar(ConfVars.HIVE_STRICT_TIMESTAMP_CONVERSION)) {
+ if (ss != null && ss.getConf().getBoolVar(ConfVars.HIVE_STRICT_CHECKS_TYPE_SAFETY)) {
Review comment:
Hey @zabetak -- had a chat with @kgyrtkirk regarding this.
Seems like udfs do not communicate their accepted format/s as they may introduce converters/Bridges/etc during initialization.
This is why HIVE-24157 introduced this restriction as part of the in the resolver
----------------------------------------------------------------
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.
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 change in pull request #1982: HIVE-24778: Unify hive.strict.timestamp.conversion and hive.strict.checks.type.safety properties
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #1982:
URL: https://github.com/apache/hive/pull/1982#discussion_r577501389
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFTimestamp.java
##########
@@ -84,12 +84,12 @@ public ObjectInspector initialize(ObjectInspector[] arguments) throws UDFArgumen
"The function TIMESTAMP takes only primitive types");
}
- if (ss != null && ss.getConf().getBoolVar(ConfVars.HIVE_STRICT_TIMESTAMP_CONVERSION)) {
+ if (ss != null && ss.getConf().getBoolVar(ConfVars.HIVE_STRICT_CHECKS_TYPE_SAFETY)) {
PrimitiveCategory category = argumentOI.getPrimitiveCategory();
PrimitiveGrouping group = PrimitiveObjectInspectorUtils.getPrimitiveGrouping(category);
if (group == PrimitiveGrouping.NUMERIC_GROUP) {
throw new UDFArgumentException(
- "Casting NUMERIC types to TIMESTAMP is prohibited (" + ConfVars.HIVE_STRICT_TIMESTAMP_CONVERSION + ")");
+ "Casting NUMERIC types to TIMESTAMP is prohibited (" + ConfVars.HIVE_STRICT_CHECKS_TYPE_SAFETY + ")");
}
Review comment:
Should this be here or rather in `TypeCheckProcFactory.DefaultExprProcessor#validateUDF`?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/udf/TimestampCastRestrictorResolver.java
##########
@@ -45,7 +45,7 @@
public TimestampCastRestrictorResolver(UDFMethodResolver parentResolver) {
this.parentResolver = parentResolver;
SessionState ss = SessionState.get();
- if (ss != null && ss.getConf().getBoolVar(ConfVars.HIVE_STRICT_TIMESTAMP_CONVERSION)) {
+ if (ss != null && ss.getConf().getBoolVar(ConfVars.HIVE_STRICT_CHECKS_TYPE_SAFETY)) {
Review comment:
If I understand well this class is used to restrict casting timestamp/date to boolean, double, byte, float, integer, long, short values. I am not sure why we should deal with these checks at this point but I if we really need this then I guess it makes sense to extend it so that we apply the same checks for all types under `hive.strict.checks.type.safety` property. Should we create another JIRA for this?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFBaseCompare.java
##########
@@ -166,10 +166,10 @@ protected void checkConversionAllowed(ObjectInspector argOI, ObjectInspector com
return;
}
SessionState ss = SessionState.get();
- if (ss != null && ss.getConf().getBoolVar(ConfVars.HIVE_STRICT_TIMESTAMP_CONVERSION)) {
+ if (ss != null && ss.getConf().getBoolVar(ConfVars.HIVE_STRICT_CHECKS_TYPE_SAFETY)) {
if (primitiveGroupOf(compareOI) == PrimitiveGrouping.NUMERIC_GROUP) {
throw new UDFArgumentException(
- "Casting DATE/TIMESTAMP to NUMERIC is prohibited (" + ConfVars.HIVE_STRICT_TIMESTAMP_CONVERSION + ")");
+ "Casting DATE/TIMESTAMP to NUMERIC is prohibited (" + ConfVars.HIVE_STRICT_CHECKS_TYPE_SAFETY + ")");
Review comment:
Why do we need this `checkConversionAllowed` method? If conversion is incompatible/dangerous shouldn't this be caught by `TypeCheckProcFactory.DefaultExprProcessor#validateUDF`?
----------------------------------------------------------------
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.
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] github-actions[bot] closed pull request #1982: HIVE-24778: Unify hive.strict.timestamp.conversion and hive.strict.checks.type.safety properties
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #1982:
URL: https://github.com/apache/hive/pull/1982
--
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.
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 change in pull request #1982: HIVE-24778: Unify hive.strict.timestamp.conversion and hive.strict.checks.type.safety properties
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #1982:
URL: https://github.com/apache/hive/pull/1982#discussion_r578457991
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/udf/TimestampCastRestrictorResolver.java
##########
@@ -45,7 +45,7 @@
public TimestampCastRestrictorResolver(UDFMethodResolver parentResolver) {
this.parentResolver = parentResolver;
SessionState ss = SessionState.get();
- if (ss != null && ss.getConf().getBoolVar(ConfVars.HIVE_STRICT_TIMESTAMP_CONVERSION)) {
+ if (ss != null && ss.getConf().getBoolVar(ConfVars.HIVE_STRICT_CHECKS_TYPE_SAFETY)) {
Review comment:
Small observation: the first example does not reflect the situation in current master. In HIVE-23100, the checks were moved from `ExprNodeGenericFuncDesc` to `TypeCheckProcFactory`.
It is true that at the moment `UDFToBoolean`, `UDFToByte`, `UDFToDouble`, etc., are not checked by `TypeCheckProcFactory`; I am not sure if this needs to change or not. In that sense if we want to enforce type checks we need to do it in another way as it is done here with the resolver.
The timestamp resolver only checks the case for timestamp/dates but there are other lossy conversions (e.g., BigInt to double) for which we are doing nothing. I assume that if somebody does `CAST( bigint as double)` the query will run without raising an error even if strict checks are enabled.
Taking it on the other way around currently if somebody does `CAST (date as double)` the query will run or fail depending on the value of the strict checks property.
----------------------------------------------------------------
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.
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