Details

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

      Description

      The elastic search adapter seems to target versions before 5.x but it would be nice to also have an adapter for ES5+ so I created one that is quite similar to the existing adapter.

        Issue Links

          Activity

          Hide
          michaelmior Michael Mior added a comment -

          Resolved in release 1.14.0 (2017-10-01)

          Show
          michaelmior Michael Mior added a comment - Resolved in release 1.14.0 (2017-10-01)
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/e1525926 ; thanks for the PR, Christian Beikov !
          Hide
          julianhyde Julian Hyde added a comment -

          New PR and JIRA case, please. I am just about to commit this one. The PR can be based on the same branch, or a new branch that starts from the old branch, I don't mind.

          Show
          julianhyde Julian Hyde added a comment - New PR and JIRA case, please. I am just about to commit this one. The PR can be based on the same branch, or a new branch that starts from the old branch, I don't mind.
          Hide
          christian.beikov Christian Beikov added a comment -

          Separate PR or can I include these changes into the existing one? Where should I put the test data file to? Or should I setup custom test data within the test that uses the same schema?

          Show
          christian.beikov Christian Beikov added a comment - Separate PR or can I include these changes into the existing one? Where should I put the test data file to? Or should I setup custom test data within the test that uses the same schema?
          Hide
          julianhyde Julian Hyde added a comment -

          I didn't even know that ES had an embedded option! Yes, that would be infinitely preferable to what we have now. We would be able to run against ES2 and ES5 in our nightly tests, and that in turn would improve quality. Can you do that as a PR? I'm reviewing your ES5 PR now.

          Show
          julianhyde Julian Hyde added a comment - I didn't even know that ES had an embedded option! Yes, that would be infinitely preferable to what we have now. We would be able to run against ES2 and ES5 in our nightly tests, and that in turn would improve quality. Can you do that as a PR? I'm reviewing your ES5 PR now.
          Hide
          christian.beikov Christian Beikov added a comment -

          I updated the PR, but there is one other thing I'd like to do if you agree it is a good idea. I'd like to use ES embedded for the tests to make them independent of an external DB. That will allow to run the tests on a CI server more easily and also allow me to do easier testing. Would that be ok or do you have a specific reason to expect an already running ES server? As an additional bonus, this will allow for testing insert/update/delete operation implementations which is something I'd like to look into in the near future.

          Show
          christian.beikov Christian Beikov added a comment - I updated the PR, but there is one other thing I'd like to do if you agree it is a good idea. I'd like to use ES embedded for the tests to make them independent of an external DB. That will allow to run the tests on a CI server more easily and also allow me to do easier testing. Would that be ok or do you have a specific reason to expect an already running ES server? As an additional bonus, this will allow for testing insert/update/delete operation implementations which is something I'd like to look into in the near future.
          Hide
          julianhyde Julian Hyde added a comment -

          That would be fine. In fact, rather than creating "elasticsearch-base", you could use the "core" module. Move the org.apache.calcite.adapter.elasticsearch package under core/src/java/main after you have abstracted any ES dependencies.

          Show
          julianhyde Julian Hyde added a comment - That would be fine. In fact, rather than creating "elasticsearch-base", you could use the "core" module. Move the org.apache.calcite.adapter.elasticsearch package under core/src/java/main after you have abstracted any ES dependencies.
          Hide
          christian.beikov Christian Beikov added a comment -

          Yeah I was thinking about depending on the original adapter too or maybe even shading it, but at first I wasn't sure how much I had to rewrite and what you would expect of a new adapter.
          I don't know if subclassing will work so easy. I can try, but I am afraid that there are some JVMs like the IBM J9 that are pretty strict about binary compatibilty i.e. errors because of non-existing statically referenced methods will be thrown during class loading vs. during first use(Oracle JVMs). And obviously, there are incompatibilities between ES2 and ES5. How about a common base module calcite-elasticsearch-base that contains the commonalities with abstract methods to override and two submodules for ES2 and ES5 that each shade the base or just normally depend on it.

          Show
          christian.beikov Christian Beikov added a comment - Yeah I was thinking about depending on the original adapter too or maybe even shading it, but at first I wasn't sure how much I had to rewrite and what you would expect of a new adapter. I don't know if subclassing will work so easy. I can try, but I am afraid that there are some JVMs like the IBM J9 that are pretty strict about binary compatibilty i.e. errors because of non-existing statically referenced methods will be thrown during class loading vs. during first use(Oracle JVMs). And obviously, there are incompatibilities between ES2 and ES5. How about a common base module calcite-elasticsearch-base that contains the commonalities with abstract methods to override and two submodules for ES2 and ES5 that each shade the base or just normally depend on it.
          Hide
          julianhyde Julian Hyde added a comment -

          Very exciting - thanks!

          Reviewing https://github.com/apache/calcite/pull/528/commits/b6864d551720b7ba734de819410f4a953896a615:

          • You seem to have started by copy-pasting the existing adapter. That's not necessarily wrong, but did you consider sub-classing the existing adapter? There are only about 50 lines different out of about 2,000 lines of code.
          • Only ElasticSearchEnumerator, ElasticSearchTable, ElasticSearchSchema depend on org.elasticsearch APIs; so could we isolate the differences between ES4 and ES5 and re-use most of the adapter?
          • We will need changes to adapter.md, elasticsearch_adapter.md, sqlline, sqlline.bat similar to CALCITE-1253

          Subhobrata Dey, as you wrote CALCITE-1253, can you please review what Christian Beikov has done here?

          Show
          julianhyde Julian Hyde added a comment - Very exciting - thanks! Reviewing https://github.com/apache/calcite/pull/528/commits/b6864d551720b7ba734de819410f4a953896a615: You seem to have started by copy-pasting the existing adapter. That's not necessarily wrong, but did you consider sub-classing the existing adapter? There are only about 50 lines different out of about 2,000 lines of code. Only ElasticSearchEnumerator, ElasticSearchTable, ElasticSearchSchema depend on org.elasticsearch APIs; so could we isolate the differences between ES4 and ES5 and re-use most of the adapter? We will need changes to adapter.md, elasticsearch_adapter.md, sqlline, sqlline.bat similar to CALCITE-1253 Subhobrata Dey , as you wrote CALCITE-1253 , can you please review what Christian Beikov has done here?
          Show
          christian.beikov Christian Beikov added a comment - See the PR https://github.com/apache/calcite/pull/528

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              christian.beikov Christian Beikov
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development