Airblader commented on a change in pull request #17256: URL: https://github.com/apache/flink/pull/17256#discussion_r707163796
########## File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/CoalesceTypeStrategy.java ########## @@ -0,0 +1,57 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.table.types.inference.strategies; + +import org.apache.flink.annotation.Internal; +import org.apache.flink.table.types.DataType; +import org.apache.flink.table.types.inference.CallContext; +import org.apache.flink.table.types.inference.TypeStrategy; +import org.apache.flink.table.types.logical.LogicalType; +import org.apache.flink.table.types.logical.utils.LogicalTypeMerging; +import org.apache.flink.table.types.utils.TypeConversions; + +import java.util.Optional; +import java.util.stream.Collectors; + +/** + * A type strategy for COALESCE return type. The return type of COALESCE is the least restrictive + * type of its arguments, and it's nullable only if all its input args are nullable. + */ +@Internal +public final class CoalesceTypeStrategy implements TypeStrategy { + + public CoalesceTypeStrategy() {} Review comment: nit: default constructor isn't needed ########## File path: flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/calcite/FlinkPlannerImpl.scala ########## @@ -97,6 +97,7 @@ class FlinkPlannerImpl( typeFactory, SqlValidator.Config.DEFAULT .withIdentifierExpansion(true) + .withCallRewrite(false) // Disables the rewrite of COALESCE Review comment: Does it _only_ do that? Did you look into whether it has other effects? ########## File path: flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/CoalesceFunctionITCase.java ########## @@ -0,0 +1,87 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.table.planner.functions; + +import org.apache.flink.table.api.DataTypes; +import org.apache.flink.table.functions.BuiltInFunctionDefinitions; + +import org.junit.runners.Parameterized; + +import java.util.Arrays; +import java.util.List; + +import static org.apache.flink.table.api.DataTypes.BIGINT; +import static org.apache.flink.table.api.DataTypes.INT; +import static org.apache.flink.table.api.DataTypes.STRING; +import static org.apache.flink.table.api.Expressions.$; +import static org.apache.flink.table.api.Expressions.call; +import static org.apache.flink.table.api.Expressions.coalesce; + +/** Test COALESCE and its return type. * */ +public class CoalesceFunctionITCase extends BuiltInFunctionTestBase { + + @Parameterized.Parameters(name = "{index}: {0}") + public static List<TestSpec> testData() { + return Arrays.asList( + TestSpec.forFunction(BuiltInFunctionDefinitions.COALESCE) + .onFieldsWithData(null, null, 1) + .andDataTypes(BIGINT().nullable(), INT().nullable(), INT().notNull()) + .testResult( + coalesce($("f0"), $("f1")), + "COALESCE(f0, f1)", + null, + DataTypes.BIGINT().nullable()) + .testResult( + coalesce($("f0"), $("f2")), + "COALESCE(f0, f2)", + 1L, + DataTypes.BIGINT().notNull()) + .testResult( + coalesce($("f1"), $("f2")), + "COALESCE(f1, f2)", + 1, + DataTypes.INT().notNull()) + .testResult( Review comment: This test case seems redundant; compared to the previous cases it just asserts that a constant is inferred as a `NOT NULL` data type, but that's not something we need to test here. ########## File path: flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/Expressions.java ########## @@ -535,6 +535,18 @@ public static ApiExpression ifThenElse(Object condition, Object ifTrue, Object i return apiCall(BuiltInFunctionDefinitions.IF, condition, ifTrue, ifFalse); } + /** + * Coalesce specifies a series of expressions, and returns the first expression whose value is + * not null. If all the expressions evaluate as null, coalesce returns a null value. + * + * <p>e.g. coalesce($("f0"), "-") leads to "-" if f0 is null Review comment: Also we should give the same documentation treatment to the function's description in the actual docs (https://ci.apache.org/projects/flink/flink-docs-release-1.13/docs/dev/table/functions/systemfunctions/). ########## File path: flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/Expressions.java ########## @@ -535,6 +535,18 @@ public static ApiExpression ifThenElse(Object condition, Object ifTrue, Object i return apiCall(BuiltInFunctionDefinitions.IF, condition, ifTrue, ifFalse); } + /** + * Coalesce specifies a series of expressions, and returns the first expression whose value is + * not null. If all the expressions evaluate as null, coalesce returns a null value. + * + * <p>e.g. coalesce($("f0"), "-") leads to "-" if f0 is null Review comment: I think we can go into detail a bit more here and explain the return type produced by this function (common type, nullability). We can also format the example part better and more consistent (see some of the other recent examples like in `BaseExpressions`) by making use of a proper code block. Having a couple examples would be good, especially ones that don't rely on external fields (like f0) but would work out of the box. ########## File path: flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/CoalesceFunctionITCase.java ########## @@ -0,0 +1,87 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.table.planner.functions; + +import org.apache.flink.table.api.DataTypes; +import org.apache.flink.table.functions.BuiltInFunctionDefinitions; + +import org.junit.runners.Parameterized; + +import java.util.Arrays; +import java.util.List; + +import static org.apache.flink.table.api.DataTypes.BIGINT; +import static org.apache.flink.table.api.DataTypes.INT; +import static org.apache.flink.table.api.DataTypes.STRING; +import static org.apache.flink.table.api.Expressions.$; +import static org.apache.flink.table.api.Expressions.call; +import static org.apache.flink.table.api.Expressions.coalesce; + +/** Test COALESCE and its return type. * */ +public class CoalesceFunctionITCase extends BuiltInFunctionTestBase { + + @Parameterized.Parameters(name = "{index}: {0}") + public static List<TestSpec> testData() { + return Arrays.asList( + TestSpec.forFunction(BuiltInFunctionDefinitions.COALESCE) + .onFieldsWithData(null, null, 1) + .andDataTypes(BIGINT().nullable(), INT().nullable(), INT().notNull()) + .testResult( + coalesce($("f0"), $("f1")), + "COALESCE(f0, f1)", + null, + DataTypes.BIGINT().nullable()) + .testResult( + coalesce($("f0"), $("f2")), + "COALESCE(f0, f2)", + 1L, + DataTypes.BIGINT().notNull()) + .testResult( + coalesce($("f1"), $("f2")), + "COALESCE(f1, f2)", + 1, + DataTypes.INT().notNull()) + .testResult( + coalesce($("f0"), 1), + "COALESCE(f0, 1)", + 1L, + // In this case, the return type is not null because we have a + // constant in the function invocation + DataTypes.BIGINT().notNull()) + .testResult( Review comment: I'm not sure we need this test either? ########## File path: flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/CoalesceFunctionITCase.java ########## @@ -0,0 +1,87 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.table.planner.functions; + +import org.apache.flink.table.api.DataTypes; +import org.apache.flink.table.functions.BuiltInFunctionDefinitions; + +import org.junit.runners.Parameterized; + +import java.util.Arrays; +import java.util.List; + +import static org.apache.flink.table.api.DataTypes.BIGINT; +import static org.apache.flink.table.api.DataTypes.INT; +import static org.apache.flink.table.api.DataTypes.STRING; +import static org.apache.flink.table.api.Expressions.$; +import static org.apache.flink.table.api.Expressions.call; +import static org.apache.flink.table.api.Expressions.coalesce; + +/** Test COALESCE and its return type. * */ +public class CoalesceFunctionITCase extends BuiltInFunctionTestBase { + + @Parameterized.Parameters(name = "{index}: {0}") + public static List<TestSpec> testData() { + return Arrays.asList( + TestSpec.forFunction(BuiltInFunctionDefinitions.COALESCE) + .onFieldsWithData(null, null, 1) + .andDataTypes(BIGINT().nullable(), INT().nullable(), INT().notNull()) + .testResult( + coalesce($("f0"), $("f1")), + "COALESCE(f0, f1)", + null, + DataTypes.BIGINT().nullable()) + .testResult( + coalesce($("f0"), $("f2")), + "COALESCE(f0, f2)", + 1L, + DataTypes.BIGINT().notNull()) + .testResult( + coalesce($("f1"), $("f2")), + "COALESCE(f1, f2)", + 1, + DataTypes.INT().notNull()) + .testResult( + coalesce($("f0"), 1), + "COALESCE(f0, 1)", + 1L, + // In this case, the return type is not null because we have a + // constant in the function invocation + DataTypes.BIGINT().notNull()) + .testResult( + coalesce($("f0")), + "COALESCE(f0)", + null, + DataTypes.BIGINT().nullable()), + TestSpec.forFunction( Review comment: Are these testing anything that isn't already tested? ########## File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/CoalesceTypeStrategy.java ########## @@ -0,0 +1,57 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.table.types.inference.strategies; + +import org.apache.flink.annotation.Internal; +import org.apache.flink.table.types.DataType; +import org.apache.flink.table.types.inference.CallContext; +import org.apache.flink.table.types.inference.TypeStrategy; +import org.apache.flink.table.types.logical.LogicalType; +import org.apache.flink.table.types.logical.utils.LogicalTypeMerging; +import org.apache.flink.table.types.utils.TypeConversions; + +import java.util.Optional; +import java.util.stream.Collectors; + +/** + * A type strategy for COALESCE return type. The return type of COALESCE is the least restrictive + * type of its arguments, and it's nullable only if all its input args are nullable. + */ +@Internal +public final class CoalesceTypeStrategy implements TypeStrategy { + + public CoalesceTypeStrategy() {} + + @Override + public Optional<DataType> inferType(CallContext callContext) { Review comment: This strategy is essentially the same as `CommonTypeStrategy` except for how nullability is determined. We could probably reuse the `CommonTypeStrategy` and instead just create a wrapping strategy that determines the nullability to reduce the duplication here? -- 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
