[jira] [Comment Edited] (CALCITE-5839) EnumerableInterpretable#StaticFieldDetector can overwrite its flag and return an incorrect result

["Ruben Q L (Jira)" <ji...@apache.org>]

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

Ruben Q L edited comment on CALCITE-5839 at 7/12/23 1:43 PM:
-------------------------------------------------------------

Credits to [~thomas.rebele] for finding this.

[~zabetak], sorry to ping you directly, but it would seem it was you who introduced the {{StaticFieldDetector}}. Is the analysis in the ticket description correct (i.e. this is a real bug)? Or am I missing something?

It would seem the code should rather be:
{code}
@Override public Void visit(final FieldDeclaration fieldDeclaration) {
  containsStaticField |= (fieldDeclaration.modifier & Modifier.STATIC) != 0;
  return containsStaticField ? null : super.visit(fieldDeclaration);
}
{code}

or alternatively maybe: 
{code}
@Override public Void visit(final FieldDeclaration fieldDeclaration) {
  if (containsStaticField) {
    return null;
  }
  containsStaticField = (fieldDeclaration.modifier & Modifier.STATIC) != 0;
  return containsStaticField ? null : super.visit(fieldDeclaration);
}
{code}

to ensure the correct result?


was (Author: rubenql):
Credits to [~thomas.rebele] for finding this.

[~zabetak], sorry to ping you directly, but it would seem it was you who introduced the {{StaticFieldDetector}}. Is the analysis in the ticket description correct (i.e. this is a real bug)? Or am I missing something?

It would seem the code should rather be:
{code}
@Override public Void visit(final FieldDeclaration fieldDeclaration) {
  containsStaticField |= (fieldDeclaration.modifier & Modifier.STATIC) != 0;
  return containsStaticField ? null : super.visit(fieldDeclaration);
}
{code}

or alternatively maybe: 
{code}
@Override public Void visit(final FieldDeclaration fieldDeclaration) {
  if (containsStaticField) {
    return null;
  }
  containsStaticField = (fieldDeclaration.modifier & Modifier.STATIC) != 0;
  return containsStaticField ? null : super.visit(fieldDeclaration);
}
{code}

oo ensure the correct result?

> EnumerableInterpretable#StaticFieldDetector can overwrite its flag and return an incorrect result
> -------------------------------------------------------------------------------------------------
>
>                 Key: CALCITE-5839
>                 URL: https://issues.apache.org/jira/browse/CALCITE-5839
>             Project: Calcite
>          Issue Type: Bug
>          Components: core
>            Reporter: Ruben Q L
>            Assignee: Ruben Q L
>            Priority: Major
>
> In EnumerableInterpretable, before using the bindable cache, it is verified whether the class contains static fields (because in that case, the cache shall not be used):
> {code}
>   static Bindable getBindable(ClassDeclaration expr, ...) {
>     ...
>     if (CalciteSystemProperty.BINDABLE_CACHE_MAX_SIZE.value() != 0) {
>       StaticFieldDetector detector = new StaticFieldDetector();
>       expr.accept(detector);
>       if (!detector.containsStaticField) {
>         return BINDABLE_CACHE.get(classBody, () ->  compileToBindable(expr.name, s, compiler));
>       }
>     }
>   ....
>   /**
>    * A visitor detecting if the Java AST contains static fields.
>    */
>   static class StaticFieldDetector extends VisitorImpl<Void> {
>     boolean containsStaticField = false;
>     @Override public Void visit(final FieldDeclaration fieldDeclaration) {
>       containsStaticField = (fieldDeclaration.modifier & Modifier.STATIC) != 0;
>       return containsStaticField ? null : super.visit(fieldDeclaration);
>     }
>   }
> {code}
> However, it seem that the {{containsStaticField}} flag in {{{StaticFieldDetector}} can be overwritten when inspecting the field declarations of the class, so that, if a non-static field is inspected after a static field, the flag will be then (re)set to false; and the final result of the shuttle will be wrong (false, when it should have been true).



--
This message was sent by Atlassian Jira
(v8.20.10#820010)