You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/12/05 04:30:58 UTC

[GitHub] [flink] LadyForest opened a new pull request, #21452: [FLINK-30282] Fix Logical type ROW lost inner field's nullability after converting to RelDataType

LadyForest opened a new pull request, #21452:
URL: https://github.com/apache/flink/pull/21452

   ## What is the purpose of the change
   
   This pull request fixes the issue that Flink SQL's logical type `Row` lost the inner field's nullability after converting to `RelDataType`. E.g. `a Row<f0 INT NOT NULL, f1 STRING>` is converted to `a Row<f0 INT, f1 STRING>`
   
   
   ## Brief change log
   -  Let`ExtendedSqlRowTypeNameSpec` pass the inner field's nullability when deriving type.
   - Specify the `StructKind.PEEK_FIELDS_NO_EXPAND` to create a struct type. This is required because in `FlinkTypeFactory#createTypeWithNullability`, only `PEEK_FIELDS_NO_EXPAND` preserves the inner field's nullability.
   
   
   ## Verifying this change
   
   This change is already covered by existing tests `SqlToOperationConverterTest#testCreateTableWithFullDataTypes`. The nullability check has been tweaked. However, the issue that the precision of the `TIME` and `Row` type's inner comment loss still exists, so the `TODO` is not removed. Beyond this, the `table.q` file also covers the change check.
   
   
   ## 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)`: no
     - The serializers: 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 introduces 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

[GitHub] [flink] flinkbot commented on pull request #21452: [FLINK-30282] Fix Logical type ROW lost inner field's nullability after converting to RelDataType

Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #21452:
URL: https://github.com/apache/flink/pull/21452#issuecomment-1336741875

   <!--
   Meta data
   {
     "version" : 1,
     "metaDataEntries" : [ {
       "hash" : "c3bdb35efe0e90698a18302794ed37e9449b5279",
       "status" : "UNKNOWN",
       "url" : "TBD",
       "triggerID" : "c3bdb35efe0e90698a18302794ed37e9449b5279",
       "triggerType" : "PUSH"
     } ]
   }-->
   ## CI report:
   
   * c3bdb35efe0e90698a18302794ed37e9449b5279 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

[GitHub] [flink] LadyForest commented on pull request #21452: [FLINK-30282] Fix Logical type ROW lost inner field's nullability after converting to RelDataType

Posted by "LadyForest (via GitHub)" <gi...@apache.org>.
LadyForest commented on PR #21452:
URL: https://github.com/apache/flink/pull/21452#issuecomment-1461375359

   One concern is that for a nullable row type, does it make sense to let the inner field not be null?
   In other words, should we not support `ROW<a INT NOT NULL, b STRING, c DOUBLE NOT NULL>`? 
   WDYT @snuyanzin 


-- 
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

[GitHub] [flink] LadyForest commented on pull request #21452: [FLINK-30282] Fix Logical type ROW lost inner field's nullability after converting to RelDataType

Posted by "LadyForest (via GitHub)" <gi...@apache.org>.
LadyForest commented on PR #21452:
URL: https://github.com/apache/flink/pull/21452#issuecomment-1459148347

   > @LadyForest this change looks reasonable however now there are some conflicts... Do you mind if I ask you to rebase?
   
   Sure. I'll do the rebase. 


-- 
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

[GitHub] [flink] LadyForest commented on pull request #21452: [FLINK-30282] Fix Logical type ROW lost inner field's nullability after converting to RelDataType

Posted by GitBox <gi...@apache.org>.
LadyForest commented on PR #21452:
URL: https://github.com/apache/flink/pull/21452#issuecomment-1337137544

   @flinkbot run azure


-- 
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

[GitHub] [flink] snuyanzin commented on pull request #21452: [FLINK-30282] Fix Logical type ROW lost inner field's nullability after converting to RelDataType

Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #21452:
URL: https://github.com/apache/flink/pull/21452#issuecomment-1457828488

   @LadyForest this change looks reasonable however now there are some conflicts...
   Do you mind if I ask you to rebase?


-- 
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

[GitHub] [flink] LadyForest commented on pull request #21452: [FLINK-30282] Fix Logical type ROW lost inner field's nullability after converting to RelDataType

Posted by GitBox <gi...@apache.org>.
LadyForest commented on PR #21452:
URL: https://github.com/apache/flink/pull/21452#issuecomment-1338630142

   @fsk119 would you spare some time to take a look?


-- 
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

[GitHub] [flink] snuyanzin commented on a diff in pull request #21452: [FLINK-30282] Fix Logical type ROW lost inner field's nullability after converting to RelDataType

Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #21452:
URL: https://github.com/apache/flink/pull/21452#discussion_r1129506976


##########
flink-table/flink-sql-client/src/test/resources/sql/table.q:
##########
@@ -964,7 +998,7 @@ CREATE TABLE IF NOT EXISTS daily_orders (
  PRIMARY KEY(dt, `user`) NOT ENFORCED
 ) PARTITIONED BY (dt) WITH (
  'connector' = 'filesystem',
- 'path' = '$VAR_BATCH_PATH',
+ 'path' = '/var/folders/xd/9dp1y4vd3h56kjkvdk426l500000gp/T/junit5781726749537647921/7bf278b4-9932-4e24-af7b-1126df4667405030013249695173150',

Review Comment:
   this i didn't get...
   where does this magic name come from?



-- 
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

[GitHub] [flink] LadyForest commented on pull request #21452: [FLINK-30282] Fix Logical type ROW lost inner field's nullability after converting to RelDataType

Posted by "LadyForest (via GitHub)" <gi...@apache.org>.
LadyForest commented on PR #21452:
URL: https://github.com/apache/flink/pull/21452#issuecomment-1584151468

   I'd like to close this PR. According to the discussion of the Calcite community, the current behavior is by design.
   See https://issues.apache.org/jira/browse/CALCITE-2464


-- 
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

[GitHub] [flink] LadyForest commented on pull request #21452: [FLINK-30282] Fix Logical type ROW lost inner field's nullability after converting to RelDataType

Posted by "LadyForest (via GitHub)" <gi...@apache.org>.
LadyForest commented on PR #21452:
URL: https://github.com/apache/flink/pull/21452#issuecomment-1576384173

   > According to SQL standards, a record's value (with structure type) is considered null if and only if all of its fields are null.
   https://issues.apache.org/jira/browse/CALCITE-3627


-- 
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

[GitHub] [flink] LadyForest closed pull request #21452: [FLINK-30282] Fix Logical type ROW lost inner field's nullability after converting to RelDataType

Posted by "LadyForest (via GitHub)" <gi...@apache.org>.
LadyForest closed pull request #21452: [FLINK-30282] Fix Logical type ROW lost inner field's nullability after converting to RelDataType
URL: https://github.com/apache/flink/pull/21452


-- 
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

[GitHub] [flink] LadyForest commented on pull request #21452: [FLINK-30282] Fix Logical type ROW lost inner field's nullability after converting to RelDataType

Posted by "LadyForest (via GitHub)" <gi...@apache.org>.
LadyForest commented on PR #21452:
URL: https://github.com/apache/flink/pull/21452#issuecomment-1460103874

   Hi @snuyanzin, could you mind taking a look? 


-- 
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

[GitHub] [flink] LadyForest commented on a diff in pull request #21452: [FLINK-30282] Fix Logical type ROW lost inner field's nullability after converting to RelDataType

Posted by "LadyForest (via GitHub)" <gi...@apache.org>.
LadyForest commented on code in PR #21452:
URL: https://github.com/apache/flink/pull/21452#discussion_r1130310752


##########
flink-table/flink-sql-client/src/test/resources/sql/table.q:
##########
@@ -964,7 +998,7 @@ CREATE TABLE IF NOT EXISTS daily_orders (
  PRIMARY KEY(dt, `user`) NOT ENFORCED
 ) PARTITIONED BY (dt) WITH (
  'connector' = 'filesystem',
- 'path' = '$VAR_BATCH_PATH',
+ 'path' = '/var/folders/xd/9dp1y4vd3h56kjkvdk426l500000gp/T/junit5781726749537647921/7bf278b4-9932-4e24-af7b-1126df4667405030013249695173150',

Review Comment:
   > this i didn't get... can you please elaborate where does this magic name come from?
   
   Good catch! `$VAR_BATCH_PATH` is defined in `CliClientITCase` and serves as a placeholder to be replaced at runtime. I overwrote this by mistake and will revert this.



-- 
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

[GitHub] [flink] snuyanzin commented on a diff in pull request #21452: [FLINK-30282] Fix Logical type ROW lost inner field's nullability after converting to RelDataType

Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #21452:
URL: https://github.com/apache/flink/pull/21452#discussion_r1129506976


##########
flink-table/flink-sql-client/src/test/resources/sql/table.q:
##########
@@ -964,7 +998,7 @@ CREATE TABLE IF NOT EXISTS daily_orders (
  PRIMARY KEY(dt, `user`) NOT ENFORCED
 ) PARTITIONED BY (dt) WITH (
  'connector' = 'filesystem',
- 'path' = '$VAR_BATCH_PATH',
+ 'path' = '/var/folders/xd/9dp1y4vd3h56kjkvdk426l500000gp/T/junit5781726749537647921/7bf278b4-9932-4e24-af7b-1126df4667405030013249695173150',

Review Comment:
   this i didn't get...
   can you  please elaborate 
   where does this magic name come from?



-- 
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

[GitHub] [flink] LadyForest commented on pull request #21452: [FLINK-30282] Fix Logical type ROW lost inner field's nullability after converting to RelDataType

Posted by GitBox <gi...@apache.org>.
LadyForest commented on PR #21452:
URL: https://github.com/apache/flink/pull/21452#issuecomment-1337441616

   @flinkbot run azure


-- 
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