[jira] [Commented] (FLINK-14296) SqlNodes in the parser module should use an Optional for optional parameters
[ https://issues.apache.org/jira/browse/FLINK-14296?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16949876#comment-16949876 ] Jiayi Liao commented on FLINK-14296: [~dwysakowicz] [~jark] Thanks for pointing this out. The PR can be reviewed now if you can spare time. > SqlNodes in the parser module should use an Optional for optional parameters > > > Key: FLINK-14296 > URL: https://issues.apache.org/jira/browse/FLINK-14296 > Project: Flink > Issue Type: Improvement > Components: Table SQL / API >Reporter: Dawid Wysakowicz >Assignee: Jiayi Liao >Priority: Major > Labels: pull-request-available > Time Spent: 10m > Remaining Estimate: 0h > > I want to suggest using Optional for optional parameters in classes such as > SqlCreateTable/SqlCreateView/SqlTableColumn etc. > Right now we must check against null at different locations e.g.: > {code} > SqlNodeList partitionKey = sqlCreateTable.getPartitionKeyList(); > if (partitionKey != null) { > partitionKeys = partitionKey > .getList() > .stream() > .map(p -> ((SqlIdentifier) p).getSimple()) > .collect(Collectors.toList()); > } > {code} -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FLINK-14296) SqlNodes in the parser module should use an Optional for optional parameters
[ https://issues.apache.org/jira/browse/FLINK-14296?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16949350#comment-16949350 ] Jiayi Liao commented on FLINK-14296: [~jark] Sure. I'll update the PR very soon. > SqlNodes in the parser module should use an Optional for optional parameters > > > Key: FLINK-14296 > URL: https://issues.apache.org/jira/browse/FLINK-14296 > Project: Flink > Issue Type: Improvement > Components: Table SQL / API >Reporter: Dawid Wysakowicz >Assignee: Jiayi Liao >Priority: Major > Labels: pull-request-available > Time Spent: 10m > Remaining Estimate: 0h > > I want to suggest using Optional for optional parameters in classes such as > SqlCreateTable/SqlCreateView/SqlTableColumn etc. > Right now we must check against null at different locations e.g.: > {code} > SqlNodeList partitionKey = sqlCreateTable.getPartitionKeyList(); > if (partitionKey != null) { > partitionKeys = partitionKey > .getList() > .stream() > .map(p -> ((SqlIdentifier) p).getSimple()) > .collect(Collectors.toList()); > } > {code} -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FLINK-14296) SqlNodes in the parser module should use an Optional for optional parameters
[ https://issues.apache.org/jira/browse/FLINK-14296?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16947578#comment-16947578 ] Jark Wu commented on FLINK-14296: - Hi [~wind_ljy], could you update the PR according to the discussion above? I assigned this issue to you. Besides, I noticed that you are using {{Optional}} for comment, however, the community recommend not use Optional as class field [1]. You can use @Nullable annotation to indicate it is a nullable field. For the {{getComment()}}, we should update the return type to {{Optional}}. [1]: https://flink.apache.org/contributing/code-style-and-quality-java.html#java-optional > SqlNodes in the parser module should use an Optional for optional parameters > > > Key: FLINK-14296 > URL: https://issues.apache.org/jira/browse/FLINK-14296 > Project: Flink > Issue Type: Improvement > Components: Table SQL / API >Reporter: Dawid Wysakowicz >Priority: Major > Labels: pull-request-available > Time Spent: 10m > Remaining Estimate: 0h > > I want to suggest using Optional for optional parameters in classes such as > SqlCreateTable/SqlCreateView/SqlTableColumn etc. > Right now we must check against null at different locations e.g.: > {code} > SqlNodeList partitionKey = sqlCreateTable.getPartitionKeyList(); > if (partitionKey != null) { > partitionKeys = partitionKey > .getList() > .stream() > .map(p -> ((SqlIdentifier) p).getSimple()) > .collect(Collectors.toList()); > } > {code} -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FLINK-14296) SqlNodes in the parser module should use an Optional for optional parameters
[ https://issues.apache.org/jira/browse/FLINK-14296?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16947521#comment-16947521 ] Dawid Wysakowicz commented on FLINK-14296: -- Sure, I'm fine with that approach. I just wanted to have a more consistent and explicit handling of nullabillity here. > SqlNodes in the parser module should use an Optional for optional parameters > > > Key: FLINK-14296 > URL: https://issues.apache.org/jira/browse/FLINK-14296 > Project: Flink > Issue Type: Improvement > Components: Table SQL / API >Reporter: Dawid Wysakowicz >Priority: Major > Labels: pull-request-available > Time Spent: 10m > Remaining Estimate: 0h > > I want to suggest using Optional for optional parameters in classes such as > SqlCreateTable/SqlCreateView/SqlTableColumn etc. > Right now we must check against null at different locations e.g.: > {code} > SqlNodeList partitionKey = sqlCreateTable.getPartitionKeyList(); > if (partitionKey != null) { > partitionKeys = partitionKey > .getList() > .stream() > .map(p -> ((SqlIdentifier) p).getSimple()) > .collect(Collectors.toList()); > } > {code} -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FLINK-14296) SqlNodes in the parser module should use an Optional for optional parameters
[ https://issues.apache.org/jira/browse/FLINK-14296?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16947516#comment-16947516 ] Jark Wu commented on FLINK-14296: - Thanks [~dwysakowicz], I think we should check not null for primaryKeyList and uniqueKeyList and maybe other fields. And add tests (DDLs with empty columns, no primary keys, etc..) to verify they are not null. And then we can check isEmpty on the lists in {{SqlCreateTable#validate}}. > SqlNodes in the parser module should use an Optional for optional parameters > > > Key: FLINK-14296 > URL: https://issues.apache.org/jira/browse/FLINK-14296 > Project: Flink > Issue Type: Improvement > Components: Table SQL / API >Reporter: Dawid Wysakowicz >Priority: Major > Labels: pull-request-available > Time Spent: 10m > Remaining Estimate: 0h > > I want to suggest using Optional for optional parameters in classes such as > SqlCreateTable/SqlCreateView/SqlTableColumn etc. > Right now we must check against null at different locations e.g.: > {code} > SqlNodeList partitionKey = sqlCreateTable.getPartitionKeyList(); > if (partitionKey != null) { > partitionKeys = partitionKey > .getList() > .stream() > .map(p -> ((SqlIdentifier) p).getSimple()) > .collect(Collectors.toList()); > } > {code} -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FLINK-14296) SqlNodes in the parser module should use an Optional for optional parameters
[ https://issues.apache.org/jira/browse/FLINK-14296?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16947486#comment-16947486 ] Dawid Wysakowicz commented on FLINK-14296: -- Sorry I made you wait for a response. I would really suggest first checking and agreeing on what is the actual contract of those properties. The ctor of e.g. SqlCreateTable ensures that only tableName and columnList are not null. The remaining fields are nullable. Moreover even in the method {{SqlCreateTable#validate}} we are checking {{columnList}} against null, so I really doubt your claim that this is always an empty {{SqlNodeList}} stands. The fact that we do not support the primary keys and unique keys is not an argument in here. If we do not support them why not remove them from SqlCreateTable? (I am not suggesting to actually remove it, just showing that the argument is invalid). Just to summarize, I'm fine with either returning an {{Optional}} or an object and not checking against null. This has to be consistent and ensured though. > SqlNodes in the parser module should use an Optional for optional parameters > > > Key: FLINK-14296 > URL: https://issues.apache.org/jira/browse/FLINK-14296 > Project: Flink > Issue Type: Improvement > Components: Table SQL / API >Reporter: Dawid Wysakowicz >Priority: Major > Labels: pull-request-available > Time Spent: 10m > Remaining Estimate: 0h > > I want to suggest using Optional for optional parameters in classes such as > SqlCreateTable/SqlCreateView/SqlTableColumn etc. > Right now we must check against null at different locations e.g.: > {code} > SqlNodeList partitionKey = sqlCreateTable.getPartitionKeyList(); > if (partitionKey != null) { > partitionKeys = partitionKey > .getList() > .stream() > .map(p -> ((SqlIdentifier) p).getSimple()) > .collect(Collectors.toList()); > } > {code} -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FLINK-14296) SqlNodes in the parser module should use an Optional for optional parameters
[ https://issues.apache.org/jira/browse/FLINK-14296?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16946423#comment-16946423 ] Jark Wu commented on FLINK-14296: - Hi [~wind_ljy], thanks for the PR, but before submitting the PR, I think we should wait for the response from Dawid who created this issue. > SqlNodes in the parser module should use an Optional for optional parameters > > > Key: FLINK-14296 > URL: https://issues.apache.org/jira/browse/FLINK-14296 > Project: Flink > Issue Type: Improvement > Components: Table SQL / API >Reporter: Dawid Wysakowicz >Priority: Major > Labels: pull-request-available > Time Spent: 10m > Remaining Estimate: 0h > > I want to suggest using Optional for optional parameters in classes such as > SqlCreateTable/SqlCreateView/SqlTableColumn etc. > Right now we must check against null at different locations e.g.: > {code} > SqlNodeList partitionKey = sqlCreateTable.getPartitionKeyList(); > if (partitionKey != null) { > partitionKeys = partitionKey > .getList() > .stream() > .map(p -> ((SqlIdentifier) p).getSimple()) > .collect(Collectors.toList()); > } > {code} -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FLINK-14296) SqlNodes in the parser module should use an Optional for optional parameters
[ https://issues.apache.org/jira/browse/FLINK-14296?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16945233#comment-16945233 ] Jiayi Liao commented on FLINK-14296: [~jark] > SqlNodes in the parser module should use an Optional for optional parameters > > > Key: FLINK-14296 > URL: https://issues.apache.org/jira/browse/FLINK-14296 > Project: Flink > Issue Type: Improvement > Components: Table SQL / API >Reporter: Dawid Wysakowicz >Priority: Major > > I want to suggest using Optional for optional parameters in classes such as > SqlCreateTable/SqlCreateView/SqlTableColumn etc. > Right now we must check against null at different locations e.g.: > {code} > SqlNodeList partitionKey = sqlCreateTable.getPartitionKeyList(); > if (partitionKey != null) { > partitionKeys = partitionKey > .getList() > .stream() > .map(p -> ((SqlIdentifier) p).getSimple()) > .collect(Collectors.toList()); > } > {code} -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FLINK-14296) SqlNodes in the parser module should use an Optional for optional parameters
[ https://issues.apache.org/jira/browse/FLINK-14296?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16945224#comment-16945224 ] Jark Wu commented on FLINK-14296: - I agree with [~wind_ljy]. {{SqlNodeList}} is a List of SqlNode. We can check {{isEmpty}} on the List instead of checking against null. > SqlNodes in the parser module should use an Optional for optional parameters > > > Key: FLINK-14296 > URL: https://issues.apache.org/jira/browse/FLINK-14296 > Project: Flink > Issue Type: Improvement > Components: Table SQL / API >Reporter: Dawid Wysakowicz >Priority: Major > > I want to suggest using Optional for optional parameters in classes such as > SqlCreateTable/SqlCreateView/SqlTableColumn etc. > Right now we must check against null at different locations e.g.: > {code} > SqlNodeList partitionKey = sqlCreateTable.getPartitionKeyList(); > if (partitionKey != null) { > partitionKeys = partitionKey > .getList() > .stream() > .map(p -> ((SqlIdentifier) p).getSimple()) > .collect(Collectors.toList()); > } > {code} -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FLINK-14296) SqlNodes in the parser module should use an Optional for optional parameters
[ https://issues.apache.org/jira/browse/FLINK-14296?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16941677#comment-16941677 ] Jiayi Liao commented on FLINK-14296: [~dawidwys] Actually the partitionKeyList / columnList / propertyList is an empty list if it's not included and we do not support primary keys and unique keys now. Maybe we should not check against null value at all? > SqlNodes in the parser module should use an Optional for optional parameters > > > Key: FLINK-14296 > URL: https://issues.apache.org/jira/browse/FLINK-14296 > Project: Flink > Issue Type: Improvement > Components: Table SQL / API >Reporter: Dawid Wysakowicz >Priority: Major > > I want to suggest using Optional for optional parameters in classes such as > SqlCreateTable/SqlCreateView/SqlTableColumn etc. > Right now we must check against null at different locations e.g.: > {code} > SqlNodeList partitionKey = sqlCreateTable.getPartitionKeyList(); > if (partitionKey != null) { > partitionKeys = partitionKey > .getList() > .stream() > .map(p -> ((SqlIdentifier) p).getSimple()) > .collect(Collectors.toList()); > } > {code} -- This message was sent by Atlassian Jira (v8.3.4#803005)