[GitHub] [flink] snuyanzin commented on a diff in pull request #21993: [FLINK-31166][table] Fix array_contains does not support null argumen…
snuyanzin commented on code in PR #21993:
URL: https://github.com/apache/flink/pull/21993#discussion_r1130032947
##
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/CollectionFunctionsITCase.java:
##
@@ -102,6 +105,16 @@ Stream getTestSetSpecs() {
"ARRAY_CONTAINS(f4, NULL)",
true,
DataTypes.BOOLEAN().nullable())
+.testResult(
+$("f5").arrayContains(lit(null,
DataTypes.INT())),
+"ARRAY_CONTAINS(f5, CAST(NULL AS INT))",
+false,
+DataTypes.BOOLEAN().notNull())
+.testResult(
+$("f5").arrayContains(lit(4,
DataTypes.INT().notNull())),
+"ARRAY_CONTAINS(f5, 4)",
+false,
+DataTypes.BOOLEAN().notNull())
Review Comment:
would be nice to have a test checking for containing `null` with `true` as
expected result
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #21993: [FLINK-31166][table] Fix array_contains does not support null argumen…
snuyanzin commented on code in PR #21993:
URL: https://github.com/apache/flink/pull/21993#discussion_r1130031361
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/ArrayElementArgumentTypeStrategy.java:
##
@@ -42,9 +42,14 @@ public Optional inferArgumentType(
CallContext callContext, int argumentPos, boolean throwOnFailure) {
final ArrayType haystackType =
(ArrayType)
callContext.getArgumentDataTypes().get(0).getLogicalType();
-final LogicalType haystackElementType = haystackType.getElementType();
final LogicalType needleType =
callContext.getArgumentDataTypes().get(argumentPos).getLogicalType();
+LogicalType haystackElementType = haystackType.getElementType();
+
+if (!haystackElementType.isNullable() && needleType.isNullable()) {
+haystackElementType = haystackType.getElementType().copy(true);
Review Comment:
```suggestion
haystackElementType = haystackElementType.copy(true);
```
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #21993: [FLINK-31166][table] Fix array_contains does not support null argumen…
snuyanzin commented on code in PR #21993:
URL: https://github.com/apache/flink/pull/21993#discussion_r1129483531
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/ArrayElementArgumentTypeStrategy.java:
##
@@ -42,7 +42,7 @@ public Optional inferArgumentType(
CallContext callContext, int argumentPos, boolean throwOnFailure) {
final ArrayType haystackType =
(ArrayType)
callContext.getArgumentDataTypes().get(0).getLogicalType();
-final LogicalType haystackElementType = haystackType.getElementType();
+final LogicalType haystackElementType =
haystackType.getElementType().copy(true);
Review Comment:
it shouldn't be always nullable.
i see 3 cases:
1. `haystackType.getElementType()` is already nullable => no need for force
nullable
2. `haystackType.getElementType()` is not nullable and `needleType` is not
nullable => no need for force nullable
3. `haystackType.getElementType()` is not nullable and `needleType` is
nullable => need for force nullable.
right now it does some not needed work for the first case and breaks the 2nd
case
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #21993: [FLINK-31166][table] Fix array_contains does not support null argumen…
snuyanzin commented on code in PR #21993:
URL: https://github.com/apache/flink/pull/21993#discussion_r1129483531
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/ArrayElementArgumentTypeStrategy.java:
##
@@ -42,7 +42,7 @@ public Optional inferArgumentType(
CallContext callContext, int argumentPos, boolean throwOnFailure) {
final ArrayType haystackType =
(ArrayType)
callContext.getArgumentDataTypes().get(0).getLogicalType();
-final LogicalType haystackElementType = haystackType.getElementType();
+final LogicalType haystackElementType =
haystackType.getElementType().copy(true);
Review Comment:
it shouldn't be always nullable.
i see 3 cases:
1. `haystackType.getElementType()` is already nullable => no need for force
nullable
2. `haystackType.getElementType()` is not nullable and `needleType` is not
nullable => no need for force nullable
3. `haystackType.getElementType()` is not nullable and `needleType` is
nullable => need for force nullable.
right now it breaks first 2 cases
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #21993: [FLINK-31166][table] Fix array_contains does not support null argumen…
snuyanzin commented on code in PR #21993:
URL: https://github.com/apache/flink/pull/21993#discussion_r1125750756
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/SpecificInputTypeStrategies.java:
##
@@ -71,7 +71,11 @@ public final class SpecificInputTypeStrategies {
/** Argument type derived from the array element type. */
public static final ArgumentTypeStrategy ARRAY_ELEMENT_ARG =
-new ArrayElementArgumentTypeStrategy();
+new ArrayElementArgumentTypeStrategy(false);
+
+/** Argument type derived from the array element type. But leaves
nullability untouched. */
Review Comment:
i guess this change could be completely removed once the issue with
`ArrayElementArgumentTypeStrategy#inferArgumentType` is solved
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #21993: [FLINK-31166][table] Fix array_contains does not support null argumen…
snuyanzin commented on code in PR #21993:
URL: https://github.com/apache/flink/pull/21993#discussion_r1125750431
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/ArrayElementArgumentTypeStrategy.java:
##
@@ -31,24 +31,28 @@
import java.util.Optional;
-import static
org.apache.flink.table.types.logical.utils.LogicalTypeCasts.supportsImplicitCast;
-
/** Specific {@link ArgumentTypeStrategy} for {@link
BuiltInFunctionDefinitions#ARRAY_CONTAINS}. */
@Internal
class ArrayElementArgumentTypeStrategy implements ArgumentTypeStrategy {
+private final boolean preserveNullability;
+
+public ArrayElementArgumentTypeStrategy(boolean preserveNullability) {
+this.preserveNullability = preserveNullability;
+}
+
@Override
public Optional inferArgumentType(
CallContext callContext, int argumentPos, boolean throwOnFailure) {
final ArrayType haystackType =
(ArrayType)
callContext.getArgumentDataTypes().get(0).getLogicalType();
final LogicalType haystackElementType = haystackType.getElementType();
-final LogicalType needleType =
-
callContext.getArgumentDataTypes().get(argumentPos).getLogicalType();
-if (supportsImplicitCast(needleType, haystackElementType)) {
Review Comment:
`haystackElementType` should be nullable or not depending on `needle` type
nullability.
Then `supportsImplicitCast` will keep working.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #21993: [FLINK-31166][table] Fix array_contains does not support null argumen…
snuyanzin commented on code in PR #21993:
URL: https://github.com/apache/flink/pull/21993#discussion_r1117821456
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/ArrayElementArgumentTypeStrategy.java:
##
@@ -31,24 +31,28 @@
import java.util.Optional;
-import static
org.apache.flink.table.types.logical.utils.LogicalTypeCasts.supportsImplicitCast;
-
/** Specific {@link ArgumentTypeStrategy} for {@link
BuiltInFunctionDefinitions#ARRAY_CONTAINS}. */
@Internal
class ArrayElementArgumentTypeStrategy implements ArgumentTypeStrategy {
+private final boolean preserveNullability;
+
+public ArrayElementArgumentTypeStrategy(boolean preserveNullability) {
+this.preserveNullability = preserveNullability;
+}
+
@Override
public Optional inferArgumentType(
CallContext callContext, int argumentPos, boolean throwOnFailure) {
final ArrayType haystackType =
(ArrayType)
callContext.getArgumentDataTypes().get(0).getLogicalType();
final LogicalType haystackElementType = haystackType.getElementType();
-final LogicalType needleType =
-
callContext.getArgumentDataTypes().get(argumentPos).getLogicalType();
-if (supportsImplicitCast(needleType, haystackElementType)) {
Review Comment:
Based on the Contract for
`org.apache.flink.table.types.inference.ArgumentTypeStrategy#inferArgumentType`
it should return an empty value in case input type could not be inferred.
Right now this change violates this contract
https://github.com/apache/flink/blob/464ded1c2a0497255b70f711167c3b7ae52ea0f7/flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/ArgumentTypeStrategy.java#L33-L48
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #21993: [FLINK-31166][table] Fix array_contains does not support null argumen…
snuyanzin commented on code in PR #21993:
URL: https://github.com/apache/flink/pull/21993#discussion_r1118400181
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/ArrayElementArgumentTypeStrategy.java:
##
@@ -31,24 +31,28 @@
import java.util.Optional;
-import static
org.apache.flink.table.types.logical.utils.LogicalTypeCasts.supportsImplicitCast;
-
/** Specific {@link ArgumentTypeStrategy} for {@link
BuiltInFunctionDefinitions#ARRAY_CONTAINS}. */
@Internal
class ArrayElementArgumentTypeStrategy implements ArgumentTypeStrategy {
+private final boolean preserveNullability;
+
+public ArrayElementArgumentTypeStrategy(boolean preserveNullability) {
+this.preserveNullability = preserveNullability;
+}
+
@Override
public Optional inferArgumentType(
CallContext callContext, int argumentPos, boolean throwOnFailure) {
final ArrayType haystackType =
(ArrayType)
callContext.getArgumentDataTypes().get(0).getLogicalType();
final LogicalType haystackElementType = haystackType.getElementType();
-final LogicalType needleType =
-
callContext.getArgumentDataTypes().get(argumentPos).getLogicalType();
-if (supportsImplicitCast(needleType, haystackElementType)) {
Review Comment:
I'm pretty sure there should be a simpler way, however i can have a look
closer to the end of the week
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #21993: [FLINK-31166][table] Fix array_contains does not support null argumen…
snuyanzin commented on code in PR #21993:
URL: https://github.com/apache/flink/pull/21993#discussion_r1117821456
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/ArrayElementArgumentTypeStrategy.java:
##
@@ -31,24 +31,28 @@
import java.util.Optional;
-import static
org.apache.flink.table.types.logical.utils.LogicalTypeCasts.supportsImplicitCast;
-
/** Specific {@link ArgumentTypeStrategy} for {@link
BuiltInFunctionDefinitions#ARRAY_CONTAINS}. */
@Internal
class ArrayElementArgumentTypeStrategy implements ArgumentTypeStrategy {
+private final boolean preserveNullability;
+
+public ArrayElementArgumentTypeStrategy(boolean preserveNullability) {
+this.preserveNullability = preserveNullability;
+}
+
@Override
public Optional inferArgumentType(
CallContext callContext, int argumentPos, boolean throwOnFailure) {
final ArrayType haystackType =
(ArrayType)
callContext.getArgumentDataTypes().get(0).getLogicalType();
final LogicalType haystackElementType = haystackType.getElementType();
-final LogicalType needleType =
-
callContext.getArgumentDataTypes().get(argumentPos).getLogicalType();
-if (supportsImplicitCast(needleType, haystackElementType)) {
Review Comment:
Based on the Contract for
`org.apache.flink.table.types.inference.ArgumentTypeStrategy#inferArgumentType`
it should return an empty value in case input type could not be inferred.
Right not this change violates this contract
https://github.com/apache/flink/blob/464ded1c2a0497255b70f711167c3b7ae52ea0f7/flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/ArgumentTypeStrategy.java#L33-L48
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #21993: [FLINK-31166][table] Fix array_contains does not support null argumen…
snuyanzin commented on code in PR #21993:
URL: https://github.com/apache/flink/pull/21993#discussion_r1117070060
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/SpecificInputTypeStrategies.java:
##
@@ -71,7 +71,11 @@ public final class SpecificInputTypeStrategies {
/** Argument type derived from the array element type. */
public static final ArgumentTypeStrategy ARRAY_ELEMENT_ARG =
-new ArrayElementArgumentTypeStrategy();
+new ArrayElementArgumentTypeStrategy(false);
+
+/** Argument type derived from the array element type. But leaves
nullability untouched. */
Review Comment:
"preserving" means keeping same.
With current implementation if an input array is an array of not null ints
and input parameter for `ArrayElementArgumentTypeStrategy` constructor is
`true` then it will force set to nullable.
So it does different thing and not preserving
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #21993: [FLINK-31166][table] Fix array_contains does not support null argumen…
snuyanzin commented on code in PR #21993:
URL: https://github.com/apache/flink/pull/21993#discussion_r1116870255
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/SpecificInputTypeStrategies.java:
##
@@ -71,7 +71,11 @@ public final class SpecificInputTypeStrategies {
/** Argument type derived from the array element type. */
public static final ArgumentTypeStrategy ARRAY_ELEMENT_ARG =
-new ArrayElementArgumentTypeStrategy();
+new ArrayElementArgumentTypeStrategy(false);
+
+/** Argument type derived from the array element type. But leaves
nullability untouched. */
Review Comment:
Compare implementation. The logic in implementation is different: in
`org.apache.flink.table.types.inference.strategies.CommonArgumentTypeStrategy#inferArgumentType`
it could preserve nullability. In your case no, that's why i raised it
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #21993: [FLINK-31166][table] Fix array_contains does not support null argumen…
snuyanzin commented on code in PR #21993:
URL: https://github.com/apache/flink/pull/21993#discussion_r1116007389
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/ArrayElementArgumentTypeStrategy.java:
##
@@ -31,24 +31,28 @@
import java.util.Optional;
-import static
org.apache.flink.table.types.logical.utils.LogicalTypeCasts.supportsImplicitCast;
-
/** Specific {@link ArgumentTypeStrategy} for {@link
BuiltInFunctionDefinitions#ARRAY_CONTAINS}. */
@Internal
class ArrayElementArgumentTypeStrategy implements ArgumentTypeStrategy {
+private final boolean preserveNullability;
+
+public ArrayElementArgumentTypeStrategy(boolean preserveNullability) {
+this.preserveNullability = preserveNullability;
+}
+
@Override
public Optional inferArgumentType(
CallContext callContext, int argumentPos, boolean throwOnFailure) {
final ArrayType haystackType =
(ArrayType)
callContext.getArgumentDataTypes().get(0).getLogicalType();
final LogicalType haystackElementType = haystackType.getElementType();
-final LogicalType needleType =
-
callContext.getArgumentDataTypes().get(argumentPos).getLogicalType();
-if (supportsImplicitCast(needleType, haystackElementType)) {
Review Comment:
I didn't get: check for implicit cast should be removed? why?
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #21993: [FLINK-31166][table] Fix array_contains does not support null argumen…
snuyanzin commented on code in PR #21993:
URL: https://github.com/apache/flink/pull/21993#discussion_r1116011807
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/SpecificInputTypeStrategies.java:
##
@@ -71,7 +71,11 @@ public final class SpecificInputTypeStrategies {
/** Argument type derived from the array element type. */
public static final ArgumentTypeStrategy ARRAY_ELEMENT_ARG =
-new ArrayElementArgumentTypeStrategy();
+new ArrayElementArgumentTypeStrategy(false);
+
+/** Argument type derived from the array element type. But leaves
nullability untouched. */
Review Comment:
that seems not true.
Example: array[1, 2, 3] which has nullability `NOT NULL` and
`ARRAY_ELEMENT_ARG_NULLABLE` makes it nullable. So it looks like it doesn't
"untouched" nullability.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #21993: [FLINK-31166][table] Fix array_contains does not support null argumen…
snuyanzin commented on code in PR #21993:
URL: https://github.com/apache/flink/pull/21993#discussion_r1116007389
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/ArrayElementArgumentTypeStrategy.java:
##
@@ -31,24 +31,28 @@
import java.util.Optional;
-import static
org.apache.flink.table.types.logical.utils.LogicalTypeCasts.supportsImplicitCast;
-
/** Specific {@link ArgumentTypeStrategy} for {@link
BuiltInFunctionDefinitions#ARRAY_CONTAINS}. */
@Internal
class ArrayElementArgumentTypeStrategy implements ArgumentTypeStrategy {
+private final boolean preserveNullability;
+
+public ArrayElementArgumentTypeStrategy(boolean preserveNullability) {
+this.preserveNullability = preserveNullability;
+}
+
@Override
public Optional inferArgumentType(
CallContext callContext, int argumentPos, boolean throwOnFailure) {
final ArrayType haystackType =
(ArrayType)
callContext.getArgumentDataTypes().get(0).getLogicalType();
final LogicalType haystackElementType = haystackType.getElementType();
-final LogicalType needleType =
-
callContext.getArgumentDataTypes().get(argumentPos).getLogicalType();
-if (supportsImplicitCast(needleType, haystackElementType)) {
Review Comment:
I didn't get: check for implicit cast should be removed?
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
