Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-1384

Add freemarker templates to parser for "ALTER SYSTEM ..." calls

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.11.0
    • Component/s: None
    • Labels:
      None

      Description

      In Phoenix, we'd like to add some Phoenix-specific commands in support of our UDF implementation. For example, we'd like to add commands like to manage the jars containing the implementation of the UDFs (see this[1] comment):

      • ALTER SYSTEM ADD JAR ...
      • ALTER SYSTEM LIST JARS

      To accomplish this, we need to add freemarker templates to parser for "ALTER SYSTEM ..." calls and then leverage these templates in Phoenix.

      [1] https://issues.apache.org/jira/browse/PHOENIX-3242?focusedCommentId=15512529&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15512529

        Issue Links

          Activity

          Hide
          jamestaylor James Taylor added a comment -

          Rajeshbabu Chintaguntla - can you take this one?

          Show
          jamestaylor James Taylor added a comment - Rajeshbabu Chintaguntla - can you take this one?
          Hide
          rajeshbabu Rajeshbabu Chintaguntla added a comment -

          James Taylor In the meeting you mentioned that Julian Hyde shared some commit link for similar work. Can you provide that?

          Show
          rajeshbabu Rajeshbabu Chintaguntla added a comment - James Taylor In the meeting you mentioned that Julian Hyde shared some commit link for similar work. Can you provide that?
          Show
          jamestaylor James Taylor added a comment - Julian Hyde 's recommendation was here: https://issues.apache.org/jira/browse/CALCITE-1383?focusedCommentId=15514427&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15514427
          Hide
          jamestaylor James Taylor added a comment -

          Gabriel Reid - this is the JIRA I mentioned during our meeting last Thursday.

          Show
          jamestaylor James Taylor added a comment - Gabriel Reid - this is the JIRA I mentioned during our meeting last Thursday.
          Hide
          julianhyde Julian Hyde added a comment - - edited

          Gabriel Reid, I have reviewed your PR. It looks good. I'm happy to commit if/when you consider this finished.

          Sudheesh Katkam, I recall that you added the SET command to the parser originally, in CALCITE-823. Can you please check that Gabriel's changes don't break the Drill parser?

          Show
          julianhyde Julian Hyde added a comment - - edited Gabriel Reid , I have reviewed your PR. It looks good. I'm happy to commit if/when you consider this finished. Sudheesh Katkam , I recall that you added the SET command to the parser originally, in CALCITE-823 . Can you please check that Gabriel's changes don't break the Drill parser?
          Hide
          gabriel.reid Gabriel Reid added a comment -

          Thanks for taking a look Julian Hyde.

          The main thing I was concerned about was the parse exception thrown if the leading "ALTER <SCOPE>" part is left off of a custom alter handler (i.e. the situation demonstrated in ExtensionSqlParserTest.testAlterSystemExtensionWithoutAlter). It seems that the automatically-generated exception is pretty lacking in useful information, and I wasn't sure if there was a better way to have a more informative error message come out of the parser. Any ideas on how that can be improved?

          Show
          gabriel.reid Gabriel Reid added a comment - Thanks for taking a look Julian Hyde . The main thing I was concerned about was the parse exception thrown if the leading "ALTER <SCOPE>" part is left off of a custom alter handler (i.e. the situation demonstrated in ExtensionSqlParserTest.testAlterSystemExtensionWithoutAlter). It seems that the automatically-generated exception is pretty lacking in useful information, and I wasn't sure if there was a better way to have a more informative error message come out of the parser. Any ideas on how that can be improved?
          Hide
          julianhyde Julian Hyde added a comment -

          Yes, that's the one thing I was planning to tinker with. We should definitely use the parser grammar rather than a Java "if", since it's a syntactic decision. I think SET and RESET (without preceding ALTER) should be one grammar rule, and ALTER should be another, and they share code. (Maybe ALTER calls SET/RESET, or maybe they both call a shared rule. Probably the latter; and SqlParserPos would be one of the parameters to that rule.)

          Show
          julianhyde Julian Hyde added a comment - Yes, that's the one thing I was planning to tinker with. We should definitely use the parser grammar rather than a Java "if", since it's a syntactic decision. I think SET and RESET (without preceding ALTER) should be one grammar rule, and ALTER should be another, and they share code. (Maybe ALTER calls SET/RESET, or maybe they both call a shared rule. Probably the latter; and SqlParserPos would be one of the parameters to that rule.)
          Hide
          gabriel.reid Gabriel Reid added a comment -

          Thanks for the pointer on that Julian Hyde – makes perfect sense and was a lot easier to do than I had originally suspected. I've updated the PR with the ALTER calling SET/RESET, removing the if statement in the parsing code. If you're ok with this, it's good to go as far as I'm concerned.

          Show
          gabriel.reid Gabriel Reid added a comment - Thanks for the pointer on that Julian Hyde – makes perfect sense and was a lot easier to do than I had originally suspected. I've updated the PR with the ALTER calling SET/RESET, removing the if statement in the parsing code. If you're ok with this, it's good to go as far as I'm concerned.
          Hide
          julianhyde Julian Hyde added a comment -

          I'm reviewing your PR now. It looks good, but I've made some minor modifications such as passing "pos" and "scope" into the "SqlUploadJarNode" method. I'll commit tomorrow afternoon, when I'm back from my overnight trip.

          Show
          julianhyde Julian Hyde added a comment - I'm reviewing your PR now. It looks good, but I've made some minor modifications such as passing "pos" and "scope" into the "SqlUploadJarNode" method. I'll commit tomorrow afternoon, when I'm back from my overnight trip.
          Hide
          julianhyde Julian Hyde added a comment -

          Gabriel Reid, I'm still hitting some issues. (It might be because I foolishly removed your exclusion of the generated-test-sources directory, because I wanted to add ExtensionSqlParserTest to the test suite.) The issue is that when I run mvn clean site, the site plugin cannot find

          [ERROR] Failed to execute goal org.apache.maven.plugins:maven-site-plugin:3.4:site (default-site) on project calcite-core: failed to get report for org.apache.maven.plugins:maven-javadoc-plugin: Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.2:compile (default-compile) on project calcite-core: Compilation failure: Compilation failure:
          [ERROR] /home/jhyde/regress/calcite/core/target/generated-test-sources/javacc/org/apache/calcite/sql/parser/extension/ExtensionSqlParserImpl.java:[4,47] cannot find symbol
          [ERROR] symbol:   class SqlUploadJarNode
          [ERROR] location: package org.apache.calcite.sql.parser.extension
          [ERROR] /home/jhyde/regress/calcite/core/target/generated-test-sources/javacc/org/apache/calcite/sql/parser/extension/ExtensionSqlParserImplTokenManager.java:[3,47] cannot find symbol
          [ERROR] symbol:   class SqlUploadJarNode
          [ERROR] location: package org.apache.calcite.sql.parser.extension
          [ERROR] /home/jhyde/regress/calcite/core/target/generated-test-sources/javacc/org/apache/calcite/sql/parser/extension/ExtensionSqlParserImpl.java:[863,31] cannot find symbol
          [ERROR] symbol:   class SqlUploadJarNode
          [ERROR] location: class org.apache.calcite.sql.parser.extension.ExtensionSqlParserImpl
          

          You can find my code in https://github.com/julianhyde/calcite/tree/1384-alter.

          Show
          julianhyde Julian Hyde added a comment - Gabriel Reid , I'm still hitting some issues. (It might be because I foolishly removed your exclusion of the generated-test-sources directory, because I wanted to add ExtensionSqlParserTest to the test suite.) The issue is that when I run mvn clean site , the site plugin cannot find [ERROR] Failed to execute goal org.apache.maven.plugins:maven-site-plugin:3.4:site (default-site) on project calcite-core: failed to get report for org.apache.maven.plugins:maven-javadoc-plugin: Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.2:compile (default-compile) on project calcite-core: Compilation failure: Compilation failure: [ERROR] /home/jhyde/regress/calcite/core/target/generated-test-sources/javacc/org/apache/calcite/sql/parser/extension/ExtensionSqlParserImpl.java:[4,47] cannot find symbol [ERROR] symbol: class SqlUploadJarNode [ERROR] location: package org.apache.calcite.sql.parser.extension [ERROR] /home/jhyde/regress/calcite/core/target/generated-test-sources/javacc/org/apache/calcite/sql/parser/extension/ExtensionSqlParserImplTokenManager.java:[3,47] cannot find symbol [ERROR] symbol: class SqlUploadJarNode [ERROR] location: package org.apache.calcite.sql.parser.extension [ERROR] /home/jhyde/regress/calcite/core/target/generated-test-sources/javacc/org/apache/calcite/sql/parser/extension/ExtensionSqlParserImpl.java:[863,31] cannot find symbol [ERROR] symbol: class SqlUploadJarNode [ERROR] location: class org.apache.calcite.sql.parser.extension.ExtensionSqlParserImpl You can find my code in https://github.com/julianhyde/calcite/tree/1384-alter .
          Hide
          gabriel.reid Gabriel Reid added a comment -

          Julian Hyde looks like it's just a question of adding the same <excludePackageNames> to the reporting invocation of the javadoc plugin in the root pom. I'll add the mini-patch that fixes your branch here.

          Show
          gabriel.reid Gabriel Reid added a comment - Julian Hyde looks like it's just a question of adding the same <excludePackageNames> to the reporting invocation of the javadoc plugin in the root pom. I'll add the mini-patch that fixes your branch here.
          Hide
          julianhyde Julian Hyde added a comment -

          Gabriel Reid, Thanks for the patch. However, even with the patch, mvn -DskipTests clean install site fails for me.

          Show
          julianhyde Julian Hyde added a comment - Gabriel Reid , Thanks for the patch. However, even with the patch, mvn -DskipTests clean install site fails for me.
          Hide
          gabriel.reid Gabriel Reid added a comment -

          Sorry about that Julian Hyde – turns out that that patch worked for mvn clean install and mvn clean site, but indeed not mvn clean install site. The main underlying problem (as far as I understand it) is that the javacc plugin adds all generated directories (including test sources) as "main" project source directories. I thought I had that handled the first time around, but apparently that wasn't the case.

          I've been able to work around it with an additional patch on your branch, see https://github.com/gabrielreid/calcite/commits/CALCITE-1384-fixups. The important change is a correct exclusion of the package that is used for extension testing when running the compiler plugin (in addition to the patch that I posted here previously). I've also changed the package name of the extension testing to be very clear that it's test-specific: org.apache.calcite.sql.parser.parserextensiontesting.

          I've successfully run all of the following maven invocations here with this setup:

          • mvn clean install
          • mvn clean site
          • mvn clean install site
          Show
          gabriel.reid Gabriel Reid added a comment - Sorry about that Julian Hyde – turns out that that patch worked for mvn clean install and mvn clean site , but indeed not mvn clean install site . The main underlying problem (as far as I understand it) is that the javacc plugin adds all generated directories (including test sources) as "main" project source directories. I thought I had that handled the first time around, but apparently that wasn't the case. I've been able to work around it with an additional patch on your branch, see https://github.com/gabrielreid/calcite/commits/CALCITE-1384-fixups . The important change is a correct exclusion of the package that is used for extension testing when running the compiler plugin (in addition to the patch that I posted here previously). I've also changed the package name of the extension testing to be very clear that it's test-specific: org.apache.calcite.sql.parser.parserextensiontesting . I've successfully run all of the following maven invocations here with this setup: mvn clean install mvn clean site mvn clean install site
          Hide
          julianhyde Julian Hyde added a comment -

          Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/fea54116.

          Gabriel Reid, Thanks for the pull request, and especially thanks for your persistence in seeing this through to completion.

          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/fea54116 . Gabriel Reid , Thanks for the pull request, and especially thanks for your persistence in seeing this through to completion.
          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.11.0 (2017-01-11).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.11.0 (2017-01-11).

            People

            • Assignee:
              gabriel.reid Gabriel Reid
              Reporter:
              jamestaylor James Taylor
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development