korlov42 commented on code in PR #4409:
URL: https://github.com/apache/ignite-3/pull/4409#discussion_r1773147765
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteTypeCoercion.java:
##########
@@ -104,75 +107,63 @@ private boolean doBinaryComparisonCoercion(SqlCallBinding
binding) {
RelDataType leftType = binding.getOperandType(0);
RelDataType rightType = binding.getOperandType(1);
- //
- // binaryComparisonCoercion that makes '1' > 1 work, may introduce
some inconsistent results
- // for dynamic parameters:
- //
- // SELECT * FROM t WHERE int_col > ?:str
- // Rejected: int and str do not have compatible type families.
- //
- // SELECT * FROM t WHERE str_col > ?:int
- // Accepted: as it adds implicit cast to 'str_col' and we end up with
- //
- // SELECT * FROM t WHERE int(str_col) = ?:int
- //
- // Which is a perfectly valid plan, but it is not what one might
expect.
+ if (!typeFamiliesAreCompatible(typeFactory, leftType, rightType)) {
+ return false;
+ }
+
validateBinaryComparisonCoercion(binding, leftType, rightType,
(IgniteSqlValidator) validator);
+ // If types are equal, no need in coercion.
if (leftType.equals(rightType)) {
- // If types are the same fallback to default rules.
- return super.binaryComparisonCoercion(binding);
- } else {
- // Otherwise find the least restrictive type among the operand
types
- // and coerce the operands to that type if such type exists.
- //
- // An example of a least restrictive type from the javadoc for
RelDataTypeFactory::leastRestrictive:
- // leastRestrictive(INT, NUMERIC(3, 2)) could be NUMERIC(12, 2)
- //
- // A least restrictive type between types of different type
families does not exist -
- // the method returns null (See
SqlTypeFactoryImpl::leastRestrictive).
- //
- RelDataType targetType =
factory.leastRestrictive(Arrays.asList(leftType, rightType));
+ return false;
+ }
- if (targetType == null) {
- // If least restrictive type does not exist fallback to
default rules.
- return super.binaryComparisonCoercion(binding);
- } else {
- boolean coerced = false;
+ // Otherwise find the least restrictive type among the operand types
+ // and coerce the operands to that type if such type exists.
+ //
+ // An example of a least restrictive type from the javadoc for
RelDataTypeFactory::leastRestrictive:
+ // leastRestrictive(INT, NUMERIC(3, 2)) could be NUMERIC(12, 2)
+ //
+ // A least restrictive type between types of different type families
does not exist -
+ // the method returns null (See SqlTypeFactoryImpl::leastRestrictive).
+ //
+ RelDataType targetType =
factory.leastRestrictive(Arrays.asList(leftType, rightType));
- if (!leftType.equals(targetType)) {
- coerced = coerceOperandType(scope, call, 0, targetType);
- }
+ if (targetType == null) {
+ return false;
+ }
- if (!rightType.equals(targetType)) {
- boolean rightCoerced = coerceOperandType(scope, call, 1,
targetType);
- coerced = coerced || rightCoerced;
- }
+ boolean coerced = false;
- return coerced;
- }
+ //noinspection SimplifiableIfStatement
+ if (!leftType.equals(targetType)) {
+ coerced = coerceOperandType(scope, call, 0, targetType);
+ }
+
+ if (!rightType.equals(targetType)) {
+ boolean rightCoerced = coerceOperandType(scope, call, 1,
targetType);
+ coerced = coerced || rightCoerced;
}
+
+ return coerced;
}
/** {@inheritDoc} */
@Override
public boolean binaryArithmeticCoercion(SqlCallBinding binding) {
- ContextStack ctxStack = contextStack.get();
Review Comment:
I tried to remove context entirely, but some test failed... Perhaps, it
would be better to clean up code in follow up ticket, current one is big enough
already. WDYT?
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
