ygerzhedovich commented on code in PR #4409:
URL: https://github.com/apache/ignite-3/pull/4409#discussion_r1771635080
##########
modules/sql-engine/src/integrationTest/sql/types/char/test_implicit_cast.test:
##########
@@ -59,27 +59,6 @@ SELECT * FROM t WHERE v > 'a'::char(1);
----
b
-statement ok
-create table tiny(v TINYINT);
-
-statement ok
-insert into tiny values('127');
-
-skipif ignite3
-# https://issues.apache.org/jira/browse/IGNITE-22444
Review Comment:
should we consider to close the ticket?
##########
modules/sql-engine/src/integrationTest/sql/function/numeric/test_type_resolution.test:
##########
@@ -32,9 +32,7 @@ SELECT 1::TINYINT + 1::DOUBLE
----
2.000000
-skipif ignite3
-# https://issues.apache.org/jira/browse/IGNITE-18677
Review Comment:
I propose to close https://issues.apache.org/jira/browse/IGNITE-18677
##########
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'm wondering if should we keep `ctxStack` or not. If not, why we have it in
another methods?
##########
modules/sql-engine/src/integrationTest/sql/types/date/test_date.test_ignore:
##########
@@ -1,73 +0,0 @@
-# name: test/sql/types/date/test_date.test
-# description: Test basic DATE functionality
-# group: [date]
-# Ignore https://issues.apache.org/jira/browse/IGNITE-15619
-# Note: these tests fail because AI3 does not allow <datetime> +/- <number>
arithmetic.
-# According to the standard only <datetime> +/- <interval> arithmetic is
allowed.
-
-statement ok
-PRAGMA enable_verification
-
-# create and insert into table
-statement ok
-CREATE TABLE dates(i DATE)
-
-statement ok
-INSERT INTO dates VALUES ('1993-08-14'), (NULL)
-
-# check that we can select dates
-query T
-SELECT * FROM dates
-----
-1993-08-14
-NULL
-
-# YEAR function
-query I
-SELECT year(i) FROM dates
-----
-1993
-NULL
-
-# check that we can convert dates to string
-query T
-SELECT cast(i AS VARCHAR) FROM dates
-----
-1993-08-14
-NULL
-
-# check that we can add days to a date
-query T
-SELECT i + 5 FROM dates
-----
-1993-08-19
-NULL
-
-# check that we can subtract days from a date
-query T
-SELECT i - 5 FROM dates
Review Comment:
soulld we keep such 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
