Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove wrong CAST AST Node in coral-hive #511

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JiaLiangC
Copy link

@JiaLiangC JiaLiangC commented Jun 21, 2024

What changes are proposed in this pull request, and why are they necessary?

There are two mechanisms in antlr v3 for building abstract syntax trees (ASTs): operators and rewrite rules.
-> Using the antlr3 rewrite symbol ^ tree construction operator

By inspecting all the AST nodes produced by the hive 1 antlr3 grammar files, we can see that KW_CAST is not used as a node in the Hive AST. The AbstractASTVisitor is designed to visit all non-DDL AST nodes, so KW_CAST needs to be removed.
I wrote a simple demo that prints the AST including the CAST syntax. It is transformed into a TOK_FUNCTION node, which validates the above points.
String src10 = "SELECT id,name,CAST(salary AS FLOAT) as salary_float FROM employees";
ASTNode root = parseDriver.parse(src10).getTree();
System.out.println(root.dump());

nil
   TOK_QUERY
      TOK_FROM
         TOK_TABREF
            TOK_TABNAME
               employees
      TOK_INSERT
         TOK_DESTINATION
            TOK_DIR
               TOK_TMP_FILE
         TOK_SELECT
            TOK_SELEXPR
               TOK_TABLE_OR_COL
                  id
            TOK_SELEXPR
               TOK_TABLE_OR_COL
                  name
            TOK_SELEXPR
               TOK_FUNCTION
                  TOK_FLOAT
                  TOK_TABLE_OR_COL
                     salary
               salary_float
   <EOF>

How was this patch tested?

Test: The ParseTreeBuilderTest.java contains tests with SQL that includes the cast method. All Hive tests have passed.
image

@wmoustafa Please help review it.

@JiaLiangC JiaLiangC changed the title remove wrong CAST astnode in coral-hive remove wrong CAST AST Node in coral-hive Jun 21, 2024
@JiaLiangC JiaLiangC changed the title remove wrong CAST AST Node in coral-hive Remove wrong CAST AST Node in coral-hive Jun 21, 2024
@wmoustafa
Copy link
Contributor

There are two mechanisms in v3 for building abstract syntax trees (ASTs): operators and rewrite rules.
-> Using the antlr3 rewrite symbol ^ tree construction operator

Could you clarify what is the relationship between this intro and the PR? Especially the current coral-hive does not use Hive 3? Also, when stating v3 it is better to state it is v3 of which project, probably Hive.

@wmoustafa
Copy link
Contributor

wmoustafa commented Jun 22, 2024

While it is a good catch, I am not sure if it is sufficient to delete it based on the current tests passing. We probably need to dig deeper to find when this token is used since there might be other test cases that depend on it. Coral uses a suite of thousands of views to validate changes against internally (at LinkedIn) and we might have to validate this through that framework. @KevinGe00 FYI.

@JiaLiangC
Copy link
Author

There are two mechanisms in v3 for building abstract syntax trees (ASTs): operators and rewrite rules.
-> Using the antlr3 rewrite symbol ^ tree construction operator

Could you clarify what is the relationship between this intro and the PR? Especially the current coral-hive does not use Hive 3? Also, when stating v3 it is better to state it is v3 of which project, probably Hive.

V3 is used by Hive1 with ANTLR v3. This fix is based on Hive1 and the description has been clarified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants