[jira] [Commented] (CALCITE-3519) `inheritPath` of `RelHint` represented by `ImmutableBitSet` may be incomplete.

["Julian Hyde (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CALCITE-3519?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16977934#comment-16977934 ] 

Julian Hyde commented on CALCITE-3519:
--------------------------------------

Reviewing {{RelHint}}'s javadoc, it's not clear what {{inheritPath}} means. It's just a list of integers. Can you improve the javadoc, maybe provide some examples? With better documentation we might have done a better job reviewing.

It's also not clear from the javadoc whether the {{listOption}} and {{kvOptions}} are supposed to contain the same information, be mutually exclusive, or be unioned together.

{{RelHint.toString()}} will throw {{NullPointerException}} if {{listOptions}} or {{kvOptions}} are null.

Lastly, the javadoc should state that {{RelHint}} is immutable.

In {{SqlHint}} it is a bit strange to have {{get}} methods that are private and called only from the constructor. Maybe change {{private List<String> getOptionAsList()}} to {{private static List<String> getOptionAsList(SqlNodeList kvOptions)}}, and similarly for {{getOptionAsKVPairs}}. Or, better, get rid of the {{optionList}} and {{optionKVPairs}} fields; they're only caching, and premature caching is bad.

Also add an assert in the constructor that it is the list-style or key-value style or empty, and not a mixture of list-style and key-value style.

> `inheritPath` of `RelHint` represented by `ImmutableBitSet` may be incomplete.
> ------------------------------------------------------------------------------
>
>                 Key: CALCITE-3519
>                 URL: https://issues.apache.org/jira/browse/CALCITE-3519
>             Project: Calcite
>          Issue Type: Bug
>          Components: core
>    Affects Versions: 1.21.0
>            Reporter: Shuo Cheng
>            Assignee: Danny Chen
>            Priority: Major
>              Labels: patch, pull-request-available
>             Fix For: 1.22.0
>
>          Time Spent: 20m
>  Remaining Estimate: 0h
>
> Taking the following SQL with hint for example: (Copying from `org.apache.calcite.test.SqlHintsConverterTest.testQueryHint `)
>  
> {code:java}
> select /*+ properties(k1='v1', k2='v2'), index(ename), no_hash_join */
> from emp e1 
> inner join dept d1 on e1.deptno = d1.deptno
> inner join emp e2 on e1.ename = e2.job{code}
>  
> rel plan with hints should be something like the following according to the hint design doc:
> {code:java}
> LogicalProject(...）([PROPERTIES inheritPath:[] options:{K1=v1, K2=v2}],[INDEX inheritPath:[] options:[ENAME]],[NO_HASH_JOIN inheritPath:[]])
>   LogicalJoin(...) ([NO_HASH_JOIN inheritPath:[0]])
>     LogicalJoin(...) (NO_HASH_JOIN inheritPath:[0, 0]])
>       LogicalTableScan(...) ([PROPERTIES inheritPath:[0, 0, 0] options:{K1=v1, K2=v2}], [INDEX inheritPath:[0, 0, 0] options:[ENAME]])
>       LogicalTableScan(...) ([PROPERTIES inheritPath:[0, 0, 1] options:{K1=v1, K2=v2}], [INDEX inheritPath:[1, 0, 0] options:[ENAME]])
>     LogicalProject(...)
>       LogicalTableScan(...) ([PROPERTIES inheritPath:[0, 1, 0] options:{K1=v1, K2=v2}], [INDEX inheritPath:[0, 1, 0] options:[ENAME]]){code}
>  
> But the expected result of  `SqlHintsConverterTest.testQueryHint` violates  the intention of Hint design doc.
>  



--
This message was sent by Atlassian Jira
(v8.3.4#803005)