Airblader commented on a change in pull request #16805:
URL: https://github.com/apache/flink/pull/16805#discussion_r699863553
##########
File path: docs/data/sql_functions.yml
##########
@@ -83,7 +83,7 @@ logical:
table: BOOLEAN1 && BOOLEAN2
description: Returns TRUE if BOOLEAN1 and BOOLEAN2 are both TRUE. Supports
three-valued logic. E.g., true && Null(BOOLEAN) returns UNKNOWN.
- sql: NOT boolean
- table: '!BOOLEAN'
+ table: BOOLEAN.not() or '!BOOLEAN' (Scala only)
Review comment:
I originally pointed this out as well (just deleted the comment
yesterday). The reason I thought this isn't necessary is because `and` and `or`
as prefix functions take arbitrarily many arguments (opposed to the postfix
ones which are unary). However, `not` is always unary, and a prefix version of
it thus doesn't seem as useful. Happy to add it if you still feel otherwise,
though.
##########
File path:
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/LogicalFunctionsITCase.java
##########
@@ -35,10 +35,104 @@
@Parameterized.Parameters(name = "{index}: {0}")
public static List<TestSpec> testData() {
List<TestSpec> cases = new ArrayList<>();
+ cases.addAll(and());
Review comment:
Yeah, I noticed there were none, which is why I had added these.
##########
File path:
flink-table/flink-table-common/src/main/java/org/apache/flink/table/functions/BuiltInFunctionDefinitions.java
##########
@@ -1488,6 +1489,20 @@
// JSON functions
//
--------------------------------------------------------------------------------------------
+ public static final BuiltInFunctionDefinition IS_JSON =
+ BuiltInFunctionDefinition.newBuilder()
+ .name("IS_JSON")
+ .kind(SCALAR)
+ .inputTypeStrategy(
+ or(
+
sequence(logical(LogicalTypeFamily.CHARACTER_STRING)),
+ sequence(
+
logical(LogicalTypeFamily.CHARACTER_STRING),
+ symbol(JsonType.class))))
+
.outputTypeStrategy(explicit(DataTypes.BOOLEAN().notNull()))
Review comment:
`IS JSON` is never nullable, it returns boolean even on null input.
##########
File path:
flink-table/flink-table-common/src/main/java/org/apache/flink/table/functions/BuiltInFunctionDefinitions.java
##########
@@ -1488,6 +1489,20 @@
// JSON functions
//
--------------------------------------------------------------------------------------------
+ public static final BuiltInFunctionDefinition IS_JSON =
+ BuiltInFunctionDefinition.newBuilder()
+ .name("IS_JSON")
+ .kind(SCALAR)
+ .inputTypeStrategy(
+ or(
+
sequence(logical(LogicalTypeFamily.CHARACTER_STRING)),
+ sequence(
+
logical(LogicalTypeFamily.CHARACTER_STRING),
+ symbol(JsonType.class))))
+
.outputTypeStrategy(explicit(DataTypes.BOOLEAN().notNull()))
Review comment:
`IS JSON` is never nullable, it returns boolean even on null input.
##########
File path:
flink-table/flink-table-common/src/main/java/org/apache/flink/table/functions/BuiltInFunctionDefinitions.java
##########
@@ -1488,6 +1489,20 @@
// JSON functions
//
--------------------------------------------------------------------------------------------
+ public static final BuiltInFunctionDefinition IS_JSON =
+ BuiltInFunctionDefinition.newBuilder()
+ .name("IS_JSON")
+ .kind(SCALAR)
+ .inputTypeStrategy(
+ or(
+
sequence(logical(LogicalTypeFamily.CHARACTER_STRING)),
+ sequence(
+
logical(LogicalTypeFamily.CHARACTER_STRING),
+ symbol(JsonType.class))))
+
.outputTypeStrategy(explicit(DataTypes.BOOLEAN().notNull()))
Review comment:
`IS JSON` is supposed to never be nullable, and always return true or
false. However, it does seem this is currently implemented incorrectly. :-/
##########
File path: flink-python/pyflink/table/tests/test_expression.py
##########
@@ -19,7 +19,7 @@
from pyflink.table import DataTypes
from pyflink.table.expression import TimeIntervalUnit, TimePointUnit,
JsonExistsOnError, \
- JsonValueOnEmptyOrError
+ JsonValueOnEmptyOrError, JsonType
Review comment:
That's where all other enums are as well. Or am I misunderstanding the
question?
##########
File path:
flink-table/flink-table-common/src/main/java/org/apache/flink/table/functions/BuiltInFunctionDefinitions.java
##########
@@ -1488,6 +1489,20 @@
// JSON functions
//
--------------------------------------------------------------------------------------------
+ public static final BuiltInFunctionDefinition IS_JSON =
+ BuiltInFunctionDefinition.newBuilder()
+ .name("IS_JSON")
+ .kind(SCALAR)
+ .inputTypeStrategy(
+ or(
+
sequence(logical(LogicalTypeFamily.CHARACTER_STRING)),
+ sequence(
+
logical(LogicalTypeFamily.CHARACTER_STRING),
+ symbol(JsonType.class))))
+
.outputTypeStrategy(explicit(DataTypes.BOOLEAN().notNull()))
Review comment:
I'll fix that as part of this PR.
##########
File path:
flink-table/flink-table-common/src/main/java/org/apache/flink/table/functions/BuiltInFunctionDefinitions.java
##########
@@ -1488,6 +1489,20 @@
// JSON functions
//
--------------------------------------------------------------------------------------------
+ public static final BuiltInFunctionDefinition IS_JSON =
+ BuiltInFunctionDefinition.newBuilder()
+ .name("IS_JSON")
+ .kind(SCALAR)
+ .inputTypeStrategy(
+ or(
Review comment:
We could, but I think the logic is pretty straight-forward here and that
way programmatic calls still work, so I would leave it as is?
##########
File path: docs/data/sql_functions.yml
##########
@@ -83,7 +83,7 @@ logical:
table: BOOLEAN1 && BOOLEAN2
description: Returns TRUE if BOOLEAN1 and BOOLEAN2 are both TRUE. Supports
three-valued logic. E.g., true && Null(BOOLEAN) returns UNKNOWN.
- sql: NOT boolean
- table: '!BOOLEAN'
+ table: BOOLEAN.not() or '!BOOLEAN' (Scala only)
Review comment:
I've added it as a fixup commit to the hotfix. Feel free to either
squash it in or discard the commit.
--
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]
