twalthr commented on a change in pull request #17985:
URL: https://github.com/apache/flink/pull/17985#discussion_r761257632
##########
File path:
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -935,6 +936,12 @@ object ScalarOperatorGens {
operand: GeneratedExpression,
targetType: LogicalType)
: GeneratedExpression = {
+
+ ctx.addReusableHeaderComment("Configuration")
+ ctx.addReusableHeaderComment("-------------")
+ ctx.addReusableHeaderComment("LegacyCastBehaviour: " +
ctx.tableConfig.getLegacyCastBehaviour)
Review comment:
use the key of the config option instead?
##########
File path:
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -935,6 +936,12 @@ object ScalarOperatorGens {
operand: GeneratedExpression,
targetType: LogicalType)
: GeneratedExpression = {
+
+ ctx.addReusableHeaderComment("Configuration")
+ ctx.addReusableHeaderComment("-------------")
+ ctx.addReusableHeaderComment("LegacyCastBehaviour: " +
ctx.tableConfig.getLegacyCastBehaviour)
Review comment:
e.g. `Using option 'table.exec.my-optoin': ...`
##########
File path:
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -935,6 +936,12 @@ object ScalarOperatorGens {
operand: GeneratedExpression,
targetType: LogicalType)
: GeneratedExpression = {
+
+ ctx.addReusableHeaderComment("Configuration")
Review comment:
let's drop those two lines. every added header comment should be self
contained otherwise you will end up with comments like:
```
Configuration
--------------
config option 1
Other comment
----------------
other comment
config option 2
```
##########
File path:
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/config/ExecutionConfigOptions.java
##########
@@ -376,6 +379,17 @@
+ "Pipelined shuffle means
data will be sent to consumer tasks once produced.")
.build());
+ // TODO: Update Description with the main upcoming change: errors thrown
vs returning null
Review comment:
We should not add too many TODOs into the code base. There is always
much to do. If at all, then with a JIRA issue attached. In this case, I would
simply drop the comment.
##########
File path:
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/TableConfig.java
##########
@@ -367,6 +367,14 @@ public void addJobParameter(String key, String value) {
getConfiguration().set(PipelineOptions.GLOBAL_JOB_PARAMETERS, params);
}
+ /** Returns whether CAST should follow the legacy behaviour or the new
one. */
+ @Deprecated
+ public boolean getLegacyCastBehaviour() {
Review comment:
The `TableConfig` class is an API class that should make frequently used
options nicer. I would like to avoid adding new methods there. We should
consult the underlying `ReadableConfig` directly.
--
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
