You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/06/04 14:20:21 UTC

[GitHub] [calcite] kramerul opened a new pull request #2426: Propagate table scan hints to JDBC

kramerul opened a new pull request #2426:
URL: https://github.com/apache/calcite/pull/2426


   We would like to use table scan hints to pass [parameters and variables to HANA views](https://help.sap.com/viewer/88fe5f56472e40cca6ef3c3dcab4855b/2.0.04/en-US/fafb3ea432e54fca9eff11648df5bccd.html).
   
   It should be possible to convert the following Calcite SQL 
   
   ```SQL
   SELECT * FROM VIEW /*+ PLACEHOLDER("$$PARAMETER_1$$"='Test') */
   ```
   
   using a special `SqlDialect` to a HANA specific statement
   
   ```SQL
   SELECT * FROM VIEW ('PLACEHOLDER' = ('$$PARAMETER_1$$', 'Test'))
   ```
   
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] danny0405 commented on a change in pull request #2426: Propagate table scan hints to JDBC

Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2426:
URL: https://github.com/apache/calcite/pull/2426#discussion_r646361600



##########
File path: core/src/test/java/org/apache/calcite/test/JdbcAdapterTest.java
##########
@@ -542,6 +550,31 @@
             + "FROM \"foodmart\".\"expense_fact\"");
   }
 
+  @Test void testTableScanHint() {
+    SqlNodeList hints = HintSqlDialectFactory.captureHints(() -> CalciteAssert
+        .model(JdbcTest.FOODMART_MODEL_WITH_HINTS)
+        .enable(CalciteAssert.DB == DatabaseInstance.HSQLDB)
+        .query("?")
+        .withRel(
+            builder -> {
+              builder.getCluster().setHintStrategies(HintStrategyTable.builder()
+                  .hintStrategy("PLACEHOLDERS", HintPredicates.TABLE_SCAN)
+                  .build());
+              return builder
+                  .scan("foodmart", "expense_fact")
+                  .hints(RelHint.builder("PLACEHOLDERS")
+                      .hintOptions(ImmutableMap.of("a", "b"))

Review comment:
       You can use this method instead `hintOption(String optionKey, String optionValue)`.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] kramerul commented on pull request #2426: Propagate table scan hints to JDBC

Posted by GitBox <gi...@apache.org>.
kramerul commented on pull request #2426:
URL: https://github.com/apache/calcite/pull/2426#issuecomment-855860544


   I will create an issue and bind it to the PR.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] kramerul commented on pull request #2426: Propagate table scan hints to JDBC

Posted by GitBox <gi...@apache.org>.
kramerul commented on pull request #2426:
URL: https://github.com/apache/calcite/pull/2426#issuecomment-855867948


   I created an issue and also changed the commit message. I'm not sure how to bind this PR to the issue. In JIRA, I added a comment. In this PR, I changed the description accordingly.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] danny0405 closed pull request #2426: [CALCITE-4640] Propagate table scan hints to JDBC

Posted by GitBox <gi...@apache.org>.
danny0405 closed pull request #2426:
URL: https://github.com/apache/calcite/pull/2426


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] kramerul commented on a change in pull request #2426: Propagate table scan hints to JDBC

Posted by GitBox <gi...@apache.org>.
kramerul commented on a change in pull request #2426:
URL: https://github.com/apache/calcite/pull/2426#discussion_r646339971



##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -525,7 +530,38 @@ private static SqlNode groupItem(List<SqlNode> groupKeys,
   /** Visits a TableScan; called by {@link #dispatch} via reflection. */
   public Result visit(TableScan e) {
     final SqlIdentifier identifier = getSqlTargetTable(e);
-    return result(identifier, ImmutableList.of(Clause.FROM), e, null);
+    SqlNode node = identifier;
+    if (e instanceof Hintable) {
+      final ImmutableList<RelHint> hints = e.getHints();
+      if (hints != null && ! hints.isEmpty()) {
+        SqlParserPos pos = identifier.getParserPosition();

Review comment:
       No this seems not to be the case. I will remove this check.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] kramerul commented on pull request #2426: [CALCITE-4640] Propagate table scan hints to JDBC

Posted by GitBox <gi...@apache.org>.
kramerul commented on pull request #2426:
URL: https://github.com/apache/calcite/pull/2426#issuecomment-856481127


   I added [the proposals](https://issues.apache.org/jira/browse/CALCITE-4640?focusedCommentId=17358773&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17358773) of @julianhyde 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] danny0405 commented on a change in pull request #2426: [CALCITE-4640] Propagate table scan hints to JDBC

Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2426:
URL: https://github.com/apache/calcite/pull/2426#discussion_r647362205



##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -525,7 +529,37 @@ private static SqlNode groupItem(List<SqlNode> groupKeys,
   /** Visits a TableScan; called by {@link #dispatch} via reflection. */
   public Result visit(TableScan e) {
     final SqlIdentifier identifier = getSqlTargetTable(e);
-    return result(identifier, ImmutableList.of(Clause.FROM), e, null);
+    final SqlNode node;
+    final ImmutableList<RelHint> hints = e.getHints();
+    if (! hints.isEmpty()) {
+      SqlParserPos pos = identifier.getParserPosition();

Review comment:
       !hints.isEmpty()




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] kramerul commented on a change in pull request #2426: Propagate table scan hints to JDBC

Posted by GitBox <gi...@apache.org>.
kramerul commented on a change in pull request #2426:
URL: https://github.com/apache/calcite/pull/2426#discussion_r646366489



##########
File path: core/src/test/java/org/apache/calcite/test/JdbcAdapterTest.java
##########
@@ -542,6 +550,31 @@
             + "FROM \"foodmart\".\"expense_fact\"");
   }
 
+  @Test void testTableScanHint() {
+    SqlNodeList hints = HintSqlDialectFactory.captureHints(() -> CalciteAssert
+        .model(JdbcTest.FOODMART_MODEL_WITH_HINTS)
+        .enable(CalciteAssert.DB == DatabaseInstance.HSQLDB)
+        .query("?")
+        .withRel(
+            builder -> {
+              builder.getCluster().setHintStrategies(HintStrategyTable.builder()
+                  .hintStrategy("PLACEHOLDERS", HintPredicates.TABLE_SCAN)
+                  .build());
+              return builder
+                  .scan("foodmart", "expense_fact")
+                  .hints(RelHint.builder("PLACEHOLDERS")
+                      .hintOptions(ImmutableMap.of("a", "b"))

Review comment:
       I will change it accordingly.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] danny0405 commented on a change in pull request #2426: Propagate table scan hints to JDBC

Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2426:
URL: https://github.com/apache/calcite/pull/2426#discussion_r646331922



##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -525,7 +530,38 @@ private static SqlNode groupItem(List<SqlNode> groupKeys,
   /** Visits a TableScan; called by {@link #dispatch} via reflection. */
   public Result visit(TableScan e) {
     final SqlIdentifier identifier = getSqlTargetTable(e);
-    return result(identifier, ImmutableList.of(Clause.FROM), e, null);
+    SqlNode node = identifier;
+    if (e instanceof Hintable) {
+      final ImmutableList<RelHint> hints = e.getHints();
+      if (hints != null && ! hints.isEmpty()) {
+        SqlParserPos pos = identifier.getParserPosition();

Review comment:
       Does the `Hintable.getHints` has any chance to return null ?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] danny0405 commented on pull request #2426: Propagate table scan hints to JDBC

Posted by GitBox <gi...@apache.org>.
danny0405 commented on pull request #2426:
URL: https://github.com/apache/calcite/pull/2426#issuecomment-855858087


   The commit message and title should in the format `[CALCITE-{JIRA_ID}] The actual message` , can you log an issue and bind this PR with that ? Also fix the commit message then.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] danny0405 commented on pull request #2426: Propagate table scan hints to JDBC

Posted by GitBox <gi...@apache.org>.
danny0405 commented on pull request #2426:
URL: https://github.com/apache/calcite/pull/2426#issuecomment-855750032


   > Hi @danny0405,
   > thanks for your review. This feature will not be used inside HANA. We are building a project, which reads analytical data from several sources using calcite. One of those sources is SAP HANA. Calcite is ideal for this type of application. It works really fine.
   
   There is a test error, seems caused by the `instanceOf` check, can you take a look ?
   
   ```java
   [BadInstanceof] `e` is an instance of TableScan which is a subtype of Hintable, so this is equivalent to a null check.
       if (e instanceof Hintable) {
             ^
       (see https://errorprone.info/bugpattern/BadInstanceof)
     Did you mean 'if (e != null) {'?
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] kramerul edited a comment on pull request #2426: Propagate table scan hints to JDBC

Posted by GitBox <gi...@apache.org>.
kramerul edited a comment on pull request #2426:
URL: https://github.com/apache/calcite/pull/2426#issuecomment-855849412


   I removed the check `(e instanceof Hintable)` since it was not required.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] kramerul commented on pull request #2426: Propagate table scan hints to JDBC

Posted by GitBox <gi...@apache.org>.
kramerul commented on pull request #2426:
URL: https://github.com/apache/calcite/pull/2426#issuecomment-855712778


   Hi @danny0405,
   thanks for your review. This feature will not be used inside HANA. We are building a project, which reads analytical data from several sources using calcite. One of those sources is SAP HANA. Calcite is ideal for this type of application. It works really fine.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] danny0405 commented on pull request #2426: Propagate table scan hints to JDBC

Posted by GitBox <gi...@apache.org>.
danny0405 commented on pull request #2426:
URL: https://github.com/apache/calcite/pull/2426#issuecomment-855704796


   Thanks for the contribution @kramerul , the code looks good, only some small suggestions, did HANA use this feature internally now ? Does it work as expected ?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] kramerul commented on a change in pull request #2426: [CALCITE-4640] Propagate table scan hints to JDBC

Posted by GitBox <gi...@apache.org>.
kramerul commented on a change in pull request #2426:
URL: https://github.com/apache/calcite/pull/2426#discussion_r647375001



##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -525,7 +529,37 @@ private static SqlNode groupItem(List<SqlNode> groupKeys,
   /** Visits a TableScan; called by {@link #dispatch} via reflection. */
   public Result visit(TableScan e) {
     final SqlIdentifier identifier = getSqlTargetTable(e);
-    return result(identifier, ImmutableList.of(Clause.FROM), e, null);
+    final SqlNode node;
+    final ImmutableList<RelHint> hints = e.getHints();
+    if (! hints.isEmpty()) {
+      SqlParserPos pos = identifier.getParserPosition();

Review comment:
       I changed this




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] kramerul commented on pull request #2426: Propagate table scan hints to JDBC

Posted by GitBox <gi...@apache.org>.
kramerul commented on pull request #2426:
URL: https://github.com/apache/calcite/pull/2426#issuecomment-855849412


   I remove the check `(e instanceof Hintable)` since it was not required.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org