[jira] [Commented] (CALCITE-4327) RelBuilder#scan(...) should work when used in RelRule#onMatch as call.builder()

[Vladimir Sitnikov (Jira)]

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

Vladimir Sitnikov commented on CALCITE-4327:
--------------------------------------------

[~julianhyde], what do you think of moving {{RelOptSchema relOptSchema}} from a 
field to {{scan}} argument?

My current code (~calcite 1.19):
{code:java}
    @Override
    public void onMatch(RelOptRuleCall call) {
        InstanceByClassTableScan scan = call.rel(0);
        RelOptTable table = scan.getTable();
        RelOptSchema schema = table.getRelOptSchema();
        List<String> indexName = new 
ArrayList<String>(table.getQualifiedName());
        indexName.set(indexName.size() - 1, "$ids$:" + 
indexName.get(indexName.size() - 1));
        LogicalTableScan ids = LogicalTableScan.create(scan.getCluster(), 
schema.getTableForMember(indexName));
...
{code}

I tried {{call.builder().scan(indexName)}} instead of {{LogicalTableScan...}}, 
however it fails with NPE as in description.

It looks like the new way to create TableScan is even worse than before:

{code:java}
    public void onMatch(RelOptRuleCall call) {
        InstanceByClassTableScan scan = call.rel(0);
        RelOptTable table = scan.getTable();
        RelOptSchema schema = table.getRelOptSchema();
        List<String> indexName = new 
ArrayList<String>(table.getQualifiedName());
        indexName.set(indexName.size() - 1, "$ids$:" + 
indexName.get(indexName.size() - 1));
        RelBuilder relBuilder = call.builder();
        relBuilder.push(
                relBuilder.getScanFactory().createScan(
                        ViewExpanders.simpleContext(relBuilder.getCluster()),
                        schema.getTableForMember(indexName)));
{code}

Then, PigRelBuilder has the following methods which suggest the need is indeed 
there:

{code:java}
  public RelBuilder scan(RelOptTable userSchema, String... tableNames) {
...
  }

  private RelBuilder scan(RelOptTable tableSchema) {
    final RelNode scan = getScanFactory().createScan(
        ViewExpanders.simpleContext(cluster), tableSchema);
    push(scan);
    return this;
  }
{code}

> RelBuilder#scan(...) should work when used in RelRule#onMatch as 
> call.builder()
> -------------------------------------------------------------------------------
>
>                 Key: CALCITE-4327
>                 URL: https://issues.apache.org/jira/browse/CALCITE-4327
>             Project: Calcite
>          Issue Type: Sub-task
>          Components: core
>    Affects Versions: 1.26.0
>            Reporter: Vladimir Sitnikov
>            Priority: Major
>
> It turns out {{RelBuilder#relOptSchema}} is often null.
> For instance,
> {code:java}
> public abstract class RelOptRuleCall {
>   public RelBuilder builder() {
>     return rule.relBuilderFactory.create(rel(0).getCluster(), /* schema= */ 
> null);
>   }
> {code}
> It leads to NPE in user code.
> I suggest either populate {{relOptSchema}} with root schema by default or 
> remove the field to avoid confusion for those who would try {{scan(...)}} 
> method.
> It might make sense to add RelOptSchema parameter to 
> {{scan(Iterable<String>)}} method as well.
> Sample use case:
> {code:java}
>     @Override
>     public void onMatch(RelOptRuleCall call) {
>         List<String> indexName = ...;
>         RelBuilder relBuilder = call.builder()
>                 .scan(indexName);
> {code}
> {noformat}
> Caused by: java.lang.NullPointerException
>       at org.apache.calcite.tools.RelBuilder.scan(RelBuilder.java:1094)
>       at 
> com.github.vlsi.mat.calcite.schema.objects.InstanceAccessByClassIdRule.onMatch(InstanceAccessByClassIdRule.java:44)
>       at 
> org.apache.calcite.plan.volcano.VolcanoRuleCall.onMatch(VolcanoRuleCall.java:229)
>       ... 75 more
> {noformat}



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