You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "LadyForest (via GitHub)" <gi...@apache.org> on 2024/06/05 04:12:14 UTC
[PR] [Flink-35473][table] Improve Table/SQL Configuration for Flink 2.0 [flink]
LadyForest opened a new pull request, #24889:
URL: https://github.com/apache/flink/pull/24889
## What is the purpose of the change
This pull request implements [FLIP-457](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=307136992&src=jira).
## Brief change log
This PR contains 6 commits.
- 1a14d7ed Move adaptive local hash agg option to ExecutionConfigOptions
- 06d11a93 Move optimizer-related options to OptimizerConfigOptions
- 35007704 Move LookupJoinHintOptions to table-api module
- 5445c3d8 Deprecate the outdated optimizer config options
- bcfd31c9 Add issue navigator to track legacy window options
- dbc98f28 Add missing annotation for ResultMode and SqlClientOptions
## Verifying this change
This change is already covered by existing tests.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: yes
- The serializer: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? not applicable
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Flink-35473][table] Improve Table/SQL Configuration for Flink 2.0 [flink]
Posted by "lincoln-lil (via GitHub)" <gi...@apache.org>.
lincoln-lil commented on code in PR #24889:
URL: https://github.com/apache/flink/pull/24889#discussion_r1631347362
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/config/LookupJoinHintOptions.java:
##########
@@ -0,0 +1,136 @@
+/*
+ * 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.api.config;
+
+import org.apache.flink.annotation.PublicEvolving;
+import org.apache.flink.configuration.ConfigOption;
+
+import org.apache.flink.shaded.guava31.com.google.common.collect.ImmutableSet;
+
+import java.time.Duration;
+import java.util.HashSet;
+import java.util.Set;
+
+import static org.apache.flink.configuration.ConfigOptions.key;
+
+/**
+ * This class holds {@link org.apache.flink.configuration.ConfigOption}s used by lookup join hint.
+ */
+@PublicEvolving
+public class LookupJoinHintOptions {
+
+ public static final ConfigOption<String> LOOKUP_TABLE =
+ key("lookup-table")
Review Comment:
I checked the testing purpose of `ConfigOptionsYamlSpecTest`, and yes it's expected to prevent public options from conforming to the yaml way, but in the case of `LookupJoinHintOptions`, it's just defines hint option string names in the query hint based on the `ConfigOption` class, and doesn't need to support the standard yaml way of configuring it like other public options in the `flink-conf.yaml`.
In addition, if we change the hint option name of the `LOOKUP_TABLE`, this is also an api change that requires additional costs when the user upgrades the job. So I prefer to add this class to `ConfigurationOptionLocator`'s exception list, WDYT?
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/config/LookupJoinHintOptions.java:
##########
@@ -0,0 +1,136 @@
+/*
+ * 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.api.config;
+
+import org.apache.flink.annotation.PublicEvolving;
+import org.apache.flink.configuration.ConfigOption;
+
+import org.apache.flink.shaded.guava31.com.google.common.collect.ImmutableSet;
+
+import java.time.Duration;
+import java.util.HashSet;
+import java.util.Set;
+
+import static org.apache.flink.configuration.ConfigOptions.key;
+
+/**
+ * This class holds {@link org.apache.flink.configuration.ConfigOption}s used by lookup join hint.
Review Comment:
How about changing it to 'This class holds hint option name definitions for LOOKUP join hints based on {@link org.apache.flink.configuration.ConfigOption}.'?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Flink-35473][table] Improve Table/SQL Configuration for Flink 2.0 [flink]
Posted by "lincoln-lil (via GitHub)" <gi...@apache.org>.
lincoln-lil commented on code in PR #24889:
URL: https://github.com/apache/flink/pull/24889#discussion_r1631322964
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/plan/rules/physical/stream/IncrementalAggregateRule.scala:
##########
@@ -155,7 +156,15 @@ class IncrementalAggregateRule
object IncrementalAggregateRule {
val INSTANCE = new IncrementalAggregateRule
- // It is a experimental config, will may be removed later.
+ /**
+ * Whether to enable incremental aggregation.
+ *
+ * @deprecated
+ * This configuration has been deprecated as part of FLIP-457. Please use
+ * [[org.apache.flink.table.api.config.OptimizerConfigOptions.TABLE_OPTIMIZER_INCREMENTAL_AGG_ENABLED]]
+ * instead.
+ */
+ @Deprecated
Review Comment:
Same as `RelNodeBlock`.
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/plan/optimize/RelNodeBlock.scala:
##########
@@ -352,7 +353,14 @@ class RelNodeBlockPlanBuilder private (tableConfig: ReadableConfig) {
object RelNodeBlockPlanBuilder {
- // It is a experimental config, will may be removed later.
+ /**
+ * Whether to treat union-all node as a breakpoint.
+ * @deprecated
+ * This configuration has been deprecated as part of FLIP-457. Please use
+ * [[org.apache.flink.table.api.config.OptimizerConfigOptions.TABLE_OPTIMIZER_UNIONALL_AS_BREAKPOINT_ENABLED]]
+ * instead.
+ */
+ @Deprecated
Review Comment:
IIUC, the proposed change 'Move the configurations to ...' of the FLIP means that we can just make the move without having to keep the old positions because we haven't changed these configuration option names, so If there are no objections, you can remove the option from the original classes.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Flink-35473][table] Improve Table/SQL Configuration for Flink 2.0 [flink]
Posted by "lincoln-lil (via GitHub)" <gi...@apache.org>.
lincoln-lil commented on code in PR #24889:
URL: https://github.com/apache/flink/pull/24889#discussion_r1633334828
##########
docs/layouts/shortcodes/generated/optimizer_config_configuration.html:
##########
@@ -65,6 +71,12 @@
<td><p>Enum</p></td>
<td>When it is `TRY_RESOLVE`, the optimizer tries to resolve the correctness issue caused by 'Non-Deterministic Updates' (NDU) in a changelog pipeline. Changelog may contain kinds of message types: Insert (I), Delete (D), Update_Before (UB), Update_After (UA). There's no NDU problem in an insert only changelog pipeline. For updates, there are three main NDU problems:<br />1. Non-deterministic functions, include scalar, table, aggregate functions, both builtin and custom ones.<br />2. LookupJoin on an evolving source<br />3. Cdc-source carries metadata fields which are system columns, not belongs to the entity data itself.<br /><br />For the first step, the optimizer automatically enables the materialization for No.2(LookupJoin) if needed, and gives the detailed error message for No.1(Non-deterministic functions) and No.3(Cdc-source with metadata) which is relatively easier to solve by changing the SQL.<br />Default value is `IGNORE`, the optimizer does no changes.<br />
<br />Possible values:<ul><li>"TRY_RESOLVE"</li><li>"IGNORE"</li></ul></td>
</tr>
+ <tr>
+ <td><h5>table.optimizer.reuse-optimize-block-with-digest-enabled</h5></td>
Review Comment:
Add 'Batch & Streaming' label.
##########
docs/layouts/shortcodes/generated/optimizer_config_configuration.html:
##########
@@ -125,5 +125,11 @@
<td>Boolean</td>
<td>If set to true, it will merge projects when converting SqlNode to RelNode.<br />Note: it is not recommended to turn on unless you are aware of possible side effects, such as causing the output of certain non-deterministic expressions to not meet expectations(see FLINK-20887).</td>
</tr>
+ <tr>
+ <td><h5>table.optimizer.union-all-as-breakpoint-enabled</h5><br> <span class="label label-primary">Streaming</span></td>
Review Comment:
Should be Batch & Streaming, see `BatchCommonSubGraphBasedOptimizer#doOptimize`
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Flink-35473][table] Improve Table/SQL Configuration for Flink 2.0 [flink]
Posted by "LadyForest (via GitHub)" <gi...@apache.org>.
LadyForest commented on code in PR #24889:
URL: https://github.com/apache/flink/pull/24889#discussion_r1632297334
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/config/LookupJoinHintOptions.java:
##########
@@ -0,0 +1,136 @@
+/*
+ * 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.api.config;
+
+import org.apache.flink.annotation.PublicEvolving;
+import org.apache.flink.configuration.ConfigOption;
+
+import org.apache.flink.shaded.guava31.com.google.common.collect.ImmutableSet;
+
+import java.time.Duration;
+import java.util.HashSet;
+import java.util.Set;
+
+import static org.apache.flink.configuration.ConfigOptions.key;
+
+/**
+ * This class holds {@link org.apache.flink.configuration.ConfigOption}s used by lookup join hint.
+ */
+@PublicEvolving
+public class LookupJoinHintOptions {
+
+ public static final ConfigOption<String> LOOKUP_TABLE =
+ key("lookup-table")
Review Comment:
It makes sense to me 👍
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Flink-35473][table] Improve Table/SQL Configuration for Flink 2.0 [flink]
Posted by "LadyForest (via GitHub)" <gi...@apache.org>.
LadyForest closed pull request #24889: [Flink-35473][table] Improve Table/SQL Configuration for Flink 2.0
URL: https://github.com/apache/flink/pull/24889
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Flink-35473][table] Improve Table/SQL Configuration for Flink 2.0 [flink]
Posted by "lincoln-lil (via GitHub)" <gi...@apache.org>.
lincoln-lil commented on PR #24889:
URL: https://github.com/apache/flink/pull/24889#issuecomment-2155055636
@LadyForest Thank you for the meticulous revisions! I've checked all the changes mentioned in the FLIP and left some comments for separate commit (haven't yet validate the generated documentations that are displaying correctly, will do it when code changes finalized).
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Flink-35473][table] Improve Table/SQL Configuration for Flink 2.0 [flink]
Posted by "lincoln-lil (via GitHub)" <gi...@apache.org>.
lincoln-lil commented on code in PR #24889:
URL: https://github.com/apache/flink/pull/24889#discussion_r1632045102
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/plan/optimize/RelNodeBlock.scala:
##########
@@ -352,7 +353,14 @@ class RelNodeBlockPlanBuilder private (tableConfig: ReadableConfig) {
object RelNodeBlockPlanBuilder {
- // It is a experimental config, will may be removed later.
+ /**
+ * Whether to treat union-all node as a breakpoint.
+ * @deprecated
+ * This configuration has been deprecated as part of FLIP-457. Please use
+ * [[org.apache.flink.table.api.config.OptimizerConfigOptions.TABLE_OPTIMIZER_UNIONALL_AS_BREAKPOINT_ENABLED]]
+ * instead.
+ */
+ @Deprecated
Review Comment:
The cautious approach makes sense. In addition to marking it deprecated we'd better also add a note that it will be removed in version 2.0.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Flink-35473][table] Improve Table/SQL Configuration for Flink 2.0 [flink]
Posted by "LadyForest (via GitHub)" <gi...@apache.org>.
LadyForest commented on code in PR #24889:
URL: https://github.com/apache/flink/pull/24889#discussion_r1631874007
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/plan/optimize/RelNodeBlock.scala:
##########
@@ -352,7 +353,14 @@ class RelNodeBlockPlanBuilder private (tableConfig: ReadableConfig) {
object RelNodeBlockPlanBuilder {
- // It is a experimental config, will may be removed later.
+ /**
+ * Whether to treat union-all node as a breakpoint.
+ * @deprecated
+ * This configuration has been deprecated as part of FLIP-457. Please use
+ * [[org.apache.flink.table.api.config.OptimizerConfigOptions.TABLE_OPTIMIZER_UNIONALL_AS_BREAKPOINT_ENABLED]]
+ * instead.
+ */
+ @Deprecated
Review Comment:
> IIUC, the proposed change 'Move the configurations to ...' of the FLIP means that we can just make the move without having to keep the old positions because we haven't changed these configuration option names, so If there are no objections, you can remove the option from the original classes.
Yes, it's the original plan. But during the [discussion](https://lists.apache.org/thread/n52smchwgr6qfg3tfvmxpdshk8obgor2), Xuannan mentioned the API compatibility guarantees. The only concern is that although there's no constraint on the Experimental API, and in theory, users should not directly rely on table-planner in their projects, we're still determining whether users rely on these variables in their code.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Flink-35473][table] Improve Table/SQL Configuration for Flink 2.0 [flink]
Posted by "LadyForest (via GitHub)" <gi...@apache.org>.
LadyForest commented on code in PR #24889:
URL: https://github.com/apache/flink/pull/24889#discussion_r1627114144
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/config/LookupJoinHintOptions.java:
##########
@@ -0,0 +1,136 @@
+/*
+ * 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.api.config;
+
+import org.apache.flink.annotation.PublicEvolving;
+import org.apache.flink.configuration.ConfigOption;
+
+import org.apache.flink.shaded.guava31.com.google.common.collect.ImmutableSet;
+
+import java.time.Duration;
+import java.util.HashSet;
+import java.util.Set;
+
+import static org.apache.flink.configuration.ConfigOptions.key;
+
+/**
+ * This class holds {@link org.apache.flink.configuration.ConfigOption}s used by lookup join hint.
+ */
+@PublicEvolving
+public class LookupJoinHintOptions {
+
+ public static final ConfigOption<String> LOOKUP_TABLE =
+ key("lookup-table")
Review Comment:
Once `LookupJoinHintOptions` becomes a `PublicEvolving` config, it must adhere to the conventions of `org.apache.flink.docs.configuration.ConfigOptionsYamlSpecTest`, where the `table` key cannot be a prefix of another key. Therefore, I changed it to `lookup-table`. Of course, this name can still be discussed further. Once it is finalized, I will update the documentation accordingly.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Flink-35473][table] Improve Table/SQL Configuration for Flink 2.0 [flink]
Posted by "LadyForest (via GitHub)" <gi...@apache.org>.
LadyForest commented on code in PR #24889:
URL: https://github.com/apache/flink/pull/24889#discussion_r1631874007
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/plan/optimize/RelNodeBlock.scala:
##########
@@ -352,7 +353,14 @@ class RelNodeBlockPlanBuilder private (tableConfig: ReadableConfig) {
object RelNodeBlockPlanBuilder {
- // It is a experimental config, will may be removed later.
+ /**
+ * Whether to treat union-all node as a breakpoint.
+ * @deprecated
+ * This configuration has been deprecated as part of FLIP-457. Please use
+ * [[org.apache.flink.table.api.config.OptimizerConfigOptions.TABLE_OPTIMIZER_UNIONALL_AS_BREAKPOINT_ENABLED]]
+ * instead.
+ */
+ @Deprecated
Review Comment:
> IIUC, the proposed change 'Move the configurations to ...' of the FLIP means that we can just make the move without having to keep the old positions because we haven't changed these configuration option names, so If there are no objections, you can remove the option from the original classes.
Yes, it's the original plan. But during the [discussion](https://lists.apache.org/thread/n52smchwgr6qfg3tfvmxpdshk8obgor2), Xuannan mentioned the API compatibility guarantees. The only concern is that although there's no constraint on the Experimental API, and in theory, users should not directly rely on table-planner in their projects, we're still not sure whether users rely on these variables in their code.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Flink-35473][table] Improve Table/SQL Configuration for Flink 2.0 [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #24889:
URL: https://github.com/apache/flink/pull/24889#issuecomment-2148823508
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "dbc98f28e5eaa59a323807132c17d909fcc6133a",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "dbc98f28e5eaa59a323807132c17d909fcc6133a",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* dbc98f28e5eaa59a323807132c17d909fcc6133a UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org