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 2020/01/09 08:08:32 UTC
[GitHub] [calcite] danny0405 opened a new pull request #1738: [CALCITE-3719]
Add hint option checker to customize the option
danny0405 opened a new pull request #1738: [CALCITE-3719] Add hint option checker to customize the option
URL: https://github.com/apache/calcite/pull/1738
validation
* Add interface HintOptionChecker to let user validate each hint's
options
* Add customizable hint error handler
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1738:
[CALCITE-3719] Add hint option checker to customize the option
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1738: [CALCITE-3719] Add hint option checker to customize the option
URL: https://github.com/apache/calcite/pull/1738#discussion_r365115875
##########
File path: core/build.gradle.kts
##########
@@ -78,7 +78,7 @@ dependencies {
testImplementation(kotlin("stdlib-jdk8"))
testImplementation(kotlin("test"))
testImplementation(kotlin("test-junit5"))
- testRuntimeOnly("org.slf4j:slf4j-log4j12")
+ testImplementation("org.slf4j:slf4j-log4j12")
Review comment:
I would choose 1, but could you help to fix that ? I'm not really familiar with Gradle, sorry for that ~
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1738:
[CALCITE-3719] Add hint option checker to customize the option
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1738: [CALCITE-3719] Add hint option checker to customize the option
URL: https://github.com/apache/calcite/pull/1738#discussion_r365098625
##########
File path: core/build.gradle.kts
##########
@@ -78,7 +78,7 @@ dependencies {
testImplementation(kotlin("stdlib-jdk8"))
testImplementation(kotlin("test"))
testImplementation(kotlin("test-junit5"))
- testRuntimeOnly("org.slf4j:slf4j-log4j12")
+ testImplementation("org.slf4j:slf4j-log4j12")
Review comment:
Why is this change?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1738: [CALCITE-3719] Add hint
option checker to customize the option
Posted by GitBox <gi...@apache.org>.
vlsi commented on issue #1738: [CALCITE-3719] Add hint option checker to customize the option
URL: https://github.com/apache/calcite/pull/1738#issuecomment-572911805
@danny0405 , how many log messages will be logged in case an invalid hint is used?
For instance: user specifies an invalid hint, then the optimizer transforms the query 100500 times. Will each transformation (assuming the rule touches the rel with the hint) log a new message saying the hint is not valid?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1738:
[CALCITE-3719] Add hint option checker to customize the option
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1738: [CALCITE-3719] Add hint option checker to customize the option
URL: https://github.com/apache/calcite/pull/1738#discussion_r365098625
##########
File path: core/build.gradle.kts
##########
@@ -78,7 +78,7 @@ dependencies {
testImplementation(kotlin("stdlib-jdk8"))
testImplementation(kotlin("test"))
testImplementation(kotlin("test-junit5"))
- testRuntimeOnly("org.slf4j:slf4j-log4j12")
+ testImplementation("org.slf4j:slf4j-log4j12")
Review comment:
Why is this change?
The issue is it makes log4j APIs accessible in the testing code, so the wrong `Logger` class appears in the auto-complete :(
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] cshuo commented on issue #1738: [CALCITE-3719] Add hint
option checker to customize the option
Posted by GitBox <gi...@apache.org>.
cshuo commented on issue #1738: [CALCITE-3719] Add hint option checker to customize the option
URL: https://github.com/apache/calcite/pull/1738#issuecomment-572915705
@vlsi From my understanding, hint validation only occurs in sqlnode to relnode
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1738:
[CALCITE-3719] Add hint option checker to customize the option
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1738: [CALCITE-3719] Add hint option checker to customize the option
URL: https://github.com/apache/calcite/pull/1738#discussion_r365033938
##########
File path: core/src/main/java/org/apache/calcite/rel/hint/HintStrategyTable.java
##########
@@ -48,15 +52,26 @@
/** Empty strategies. */
// Need to replace the EMPTY with DEFAULT if we have any hint implementations.
- public static final HintStrategyTable EMPTY = new HintStrategyTable(new HashMap<>());
+ public static final HintStrategyTable EMPTY = new HintStrategyTable(
+ new HashMap<>(), new HashMap<>(), HintErrorLogger.INSTANCE);
Review comment:
I agree, thanks.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on issue #1738: [CALCITE-3719] Add
hint option checker to customize the option
Posted by GitBox <gi...@apache.org>.
danny0405 commented on issue #1738: [CALCITE-3719] Add hint option checker to customize the option
URL: https://github.com/apache/calcite/pull/1738#issuecomment-572915550
> @danny0405 , how many log messages will be logged in case an invalid hint is used?
>
> For instance: user specifies an invalid hint, then the optimizer transforms the query 100500 times. Will each transformation (assuming the rule touches the rel with the hint) log a new message saying the hint is not valid?
Each hint only once. Say if a have 10 invalid hint, there would log 10 warnings.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1738:
[CALCITE-3719] Add hint option checker to customize the option
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1738: [CALCITE-3719] Add hint option checker to customize the option
URL: https://github.com/apache/calcite/pull/1738#discussion_r365107496
##########
File path: core/build.gradle.kts
##########
@@ -78,7 +78,7 @@ dependencies {
testImplementation(kotlin("stdlib-jdk8"))
testImplementation(kotlin("test"))
testImplementation(kotlin("test-junit5"))
- testRuntimeOnly("org.slf4j:slf4j-log4j12")
+ testImplementation("org.slf4j:slf4j-log4j12")
Review comment:
Oops, i need to reference log4j class in my Test, can we fix that `auto-complete` ?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1738:
[CALCITE-3719] Add hint option checker to customize the option
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1738: [CALCITE-3719] Add hint option checker to customize the option
URL: https://github.com/apache/calcite/pull/1738#discussion_r365112920
##########
File path: core/build.gradle.kts
##########
@@ -78,7 +78,7 @@ dependencies {
testImplementation(kotlin("stdlib-jdk8"))
testImplementation(kotlin("test"))
testImplementation(kotlin("test-junit5"))
- testRuntimeOnly("org.slf4j:slf4j-log4j12")
+ testImplementation("org.slf4j:slf4j-log4j12")
Review comment:
In any case you don't really need `slf4j-log4j12` on the compilation classpath.
You don't really reference slf4j-log4j12 classes.
You reference log4j12 classes.
I see the following options:
a) Keep `slf4j-log4j12` runtime-only and add an extra `testImplementation("log4j:log4j") { because("SqlHintsConverterTest needs to implement a MockAppender") }`
b) Create an extra `src/test-logging` folder that would have extra dependency for testing logging behavior. For instance, `slf4j-test` or `spf4j-slf4j-test` which are `slf4j` implementations designed to be used for testing logging behaviors.
Then all the tests that do need to verify logging can be placed there.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 merged pull request #1738: [CALCITE-3719] Add
hint option checker to customize the option
Posted by GitBox <gi...@apache.org>.
danny0405 merged pull request #1738: [CALCITE-3719] Add hint option checker to customize the option
URL: https://github.com/apache/calcite/pull/1738
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 edited a comment on issue #1738: [CALCITE-3719]
Add hint option checker to customize the option
Posted by GitBox <gi...@apache.org>.
danny0405 edited a comment on issue #1738: [CALCITE-3719] Add hint option checker to customize the option
URL: https://github.com/apache/calcite/pull/1738#issuecomment-572915550
> @danny0405 , how many log messages will be logged in case an invalid hint is used?
>
> For instance: user specifies an invalid hint, then the optimizer transforms the query 100500 times. Will each transformation (assuming the rule touches the rel with the hint) log a new message saying the hint is not valid?
Each hint only once. Say if i have 10 invalid hints, there would log 10 warnings.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] cshuo commented on a change in pull request #1738:
[CALCITE-3719] Add hint option checker to customize the option
Posted by GitBox <gi...@apache.org>.
cshuo commented on a change in pull request #1738: [CALCITE-3719] Add hint option checker to customize the option
URL: https://github.com/apache/calcite/pull/1738#discussion_r364725783
##########
File path: core/src/main/java/org/apache/calcite/rel/hint/HintStrategyTable.java
##########
@@ -48,15 +52,26 @@
/** Empty strategies. */
// Need to replace the EMPTY with DEFAULT if we have any hint implementations.
- public static final HintStrategyTable EMPTY = new HintStrategyTable(new HashMap<>());
+ public static final HintStrategyTable EMPTY = new HintStrategyTable(
+ new HashMap<>(), new HashMap<>(), HintErrorLogger.INSTANCE);
Review comment:
`new HashMap<>()` -> `Collections.emptyMap()` may be better? I think `EMPTY` here implicate that it's an immutable object.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] cshuo commented on issue #1738: [CALCITE-3719] Add hint
option checker to customize the option
Posted by GitBox <gi...@apache.org>.
cshuo commented on issue #1738: [CALCITE-3719] Add hint option checker to customize the option
URL: https://github.com/apache/calcite/pull/1738#issuecomment-572835917
> > Hi, Danny, LGTM overall.
> > And just some thoughts, IMO, hint strategy is some kind of a `validation`, so I'm wondering if we can squash these two interface into one?
>
> I would rather keep the 2 interfaces for 3 reasons:
>
> 1. Now the `HintStrategy` can be used to multiple hints, i.e. the `NodeTypeHintStrategy`, but the `HintOptionChecker` is always used to a specific hint
> 2. The `HintOptionChecker` is optional
> 3. They can be used as functional interface, write as lambda `(rel, hint) ->`
make sense. 😃
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on issue #1738: [CALCITE-3719] Add
hint option checker to customize the option
Posted by GitBox <gi...@apache.org>.
amaliujia commented on issue #1738: [CALCITE-3719] Add hint option checker to customize the option
URL: https://github.com/apache/calcite/pull/1738#issuecomment-572708798
Looks Good. Thanks for detailed java doc to help understand this 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
With regards,
Apache Git Services
[GitHub] [calcite] cshuo commented on a change in pull request #1738:
[CALCITE-3719] Add hint option checker to customize the option
Posted by GitBox <gi...@apache.org>.
cshuo commented on a change in pull request #1738: [CALCITE-3719] Add hint option checker to customize the option
URL: https://github.com/apache/calcite/pull/1738#discussion_r364702211
##########
File path: core/src/main/java/org/apache/calcite/plan/ViewExpanders.java
##########
@@ -39,9 +39,9 @@ private ViewExpanders() {}
RelOptCluster cluster,
List<RelHint> hints) {
// See if the user want to customize the ToRelContext.
Review comment:
want -> wants.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on issue #1738: [CALCITE-3719] Add
hint option checker to customize the option
Posted by GitBox <gi...@apache.org>.
danny0405 commented on issue #1738: [CALCITE-3719] Add hint option checker to customize the option
URL: https://github.com/apache/calcite/pull/1738#issuecomment-572828858
> Hi, Danny, LGTM overall.
> And just some thoughts, IMO, hint strategy is some kind of a `validation`, so I'm wondering if we can squash these two interface into one?
I would rather keep the 2 interfaces for 3 reasons:
1. Now the `HintStrategy` can be used to multiple hints, i.e. the `NodeTypeHintStrategy`, but the `HintOptionChecker` is always used to a specific hint
2. The `HintOptionChecker` is optional
3. They can be used as functional interface, write as lambda `(rel, hint) ->`
----------------------------------------------------------------
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
With regards,
Apache Git Services