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/11/17 22:34:04 UTC
[GitHub] [calcite] asolimando opened a new pull request #2267: Following [CALCITE-4354], added 'calcite.experimental.allow.field.ind…
asolimando opened a new pull request #2267:
URL: https://github.com/apache/calcite/pull/2267
…ex.access' feature flag
Index-based field access for struct is experimental as it relies on field order which is JVM-dependent (see CALCITE-2489) and is disabled by default, it can be activated via 'calcite.experimental.allow.field.index.access=true'.
----------------------------------------------------------------
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] zabetak commented on a change in pull request #2267: Following [CALCITE-4354], added 'calcite.experimental.allow.field.ind…
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2267:
URL: https://github.com/apache/calcite/pull/2267#discussion_r525938001
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java
##########
@@ -131,6 +131,15 @@
public static final CalciteSystemProperty<Boolean> TOPDOWN_OPT =
booleanProperty("calcite.planner.topdown.opt", false);
+ /**
+ * Whether to enable index-based access for struct fields.
+ *
+ * <p>Note: the feature is experimental as it relies on field order which is JVM-dependent
+ * (see CALCITE-2489).</p>
+ */
+ public static final CalciteSystemProperty<Boolean> ALLOW_FIELD_INDEX_ACCESS =
+ booleanProperty("calcite.experimental.allow.field.index.access", false);
Review comment:
To make things more uniform with the properties above, I would rename it to: `calcite.enable.enumerable.fieldIndexAccess`. I added enumerable since it is strictly a property affecting the runtime not for parser, validator, etc.
----------------------------------------------------------------
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] zabetak closed pull request #2267: Following [CALCITE-4354], added 'calcite.experimental.allow.field.ind…
Posted by GitBox <gi...@apache.org>.
zabetak closed pull request #2267:
URL: https://github.com/apache/calcite/pull/2267
----------------------------------------------------------------
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] asolimando commented on pull request #2267: Following [CALCITE-4354], added 'calcite.experimental.allow.field.ind…
Posted by GitBox <gi...@apache.org>.
asolimando commented on pull request #2267:
URL: https://github.com/apache/calcite/pull/2267#issuecomment-729599150
> One minor comment would be to see if we can enable the property by default during tests so that we have better coverage. I guess it should be doable to pass this to the gradle scripts somewhere. I can check it out when I will merge the PR.
Thanks, that's indeed a good idea. So far none of the CI-profiles were failing, so I guess we can activate the feature flag for all of them, and disable it in the feature we add a JVM with the problematic symptoms.
----------------------------------------------------------------
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] asolimando commented on a change in pull request #2267: Following [CALCITE-4354], added 'calcite.experimental.allow.field.ind…
Posted by GitBox <gi...@apache.org>.
asolimando commented on a change in pull request #2267:
URL: https://github.com/apache/calcite/pull/2267#discussion_r526006792
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java
##########
@@ -131,6 +131,15 @@
public static final CalciteSystemProperty<Boolean> TOPDOWN_OPT =
booleanProperty("calcite.planner.topdown.opt", false);
+ /**
+ * Whether to enable index-based access for struct fields.
+ *
+ * <p>Note: the feature is experimental as it relies on field order which is JVM-dependent
+ * (see CALCITE-2489).</p>
+ */
+ public static final CalciteSystemProperty<Boolean> ALLOW_FIELD_INDEX_ACCESS =
+ booleanProperty("calcite.experimental.allow.field.index.access", false);
Review comment:
I agree on the property renaming, in light of the new name I have also moved it in the same group as the other "calcite.enable.enumerable.*" ones.
----------------------------------------------------------------
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] asolimando commented on pull request #2267: Following [CALCITE-4354], added 'calcite.experimental.allow.field.ind…
Posted by GitBox <gi...@apache.org>.
asolimando commented on pull request #2267:
URL: https://github.com/apache/calcite/pull/2267#issuecomment-729655799
There is a test failure for the last commit: https://ci.appveyor.com/project/ApacheSoftwareFoundation/calcite/builds/36376813/job/5k2lubas8qbehk8o
But it looks unrelated to me:
> GeodeBookstoreTest STANDARD_ERROR
FATAL LoggingUncaughtExceptionHandler Uncaught exception in thread Thread[ServerLauncherStopper,5,main]
java.lang.NullPointerException
at org.apache.geode.distributed.ServerLauncher.doStopInProcess(ServerLauncher.java:1253)
at java.base/java.lang.Thread.run(Thread.java:830)
FAILURE 0.0sec, org.apache.calcite.adapter.geode.rel.GeodeBookstoreTest > initializationError
java.lang.IllegalStateException: Expected state not responding but got online
at com.google.common.base.Preconditions.checkState(Preconditions.java:824)
at org.apache.calcite.adapter.geode.rel.GeodeEmbeddedPolicy.requireStatus(GeodeEmbeddedPolicy.java:94)
at org.apache.calcite.adapter.geode.rel.GeodeEmbeddedPolicy.beforeAll(GeodeEmbeddedPolicy.java:52)
at org.apache.calcite.adapter.geode.rel.GeodeEmbeddedPolicy$RefCountPolicy.beforeAll(GeodeEmbeddedPolicy.java:138)
at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeBeforeAllCallbacks$7(ClassBasedTestDescriptor.java:355)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.invokeBeforeAllCallbacks(ClassBasedTestDescriptor.java:355)
at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.before(ClassBasedTestDescriptor.java:189)
at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.before(ClassBasedTestDescriptor.java:77)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:132)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80)
FAILURE 1.4sec, 1 completed, 1 failed, 0 skipped, org.apache.calcite.adapter.geode.rel.GeodeBookstoreTest
WARNING 47.4sec, 14 completed, 0 failed, 3 skipped, org.apache.calcite.adapter.geode.rel.GeodeZipsTest
FAILURE 57.4sec, 27 completed, 1 failed, 3 skipped, Gradle Test Run :geode:test
27 tests completed, 1 failed, 3 skipped
I am pretty sure I saw a "relaunch test" button somewhere sparing the need for an empty commit to retrigger tests, but I can't find it anymore.
----------------------------------------------------------------
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] asolimando edited a comment on pull request #2267: Following [CALCITE-4354], added 'calcite.experimental.allow.field.ind…
Posted by GitBox <gi...@apache.org>.
asolimando edited a comment on pull request #2267:
URL: https://github.com/apache/calcite/pull/2267#issuecomment-729599150
> One minor comment would be to see if we can enable the property by default during tests so that we have better coverage. I guess it should be doable to pass this to the gradle scripts somewhere. I can check it out when I will merge the PR.
Thanks, that's indeed a good idea. So far none of the CI-profiles were failing, so I guess we can activate the feature flag for all of them, and disable it in the feature we add a JVM with the problematic symptoms.
Not sure where the best place to activate the flag is, but `systemProperty("calcite.enable.enumerable.fieldIndexAccess", true)` looks like the way to do it in Calcite's gradle scripts.
----------------------------------------------------------------
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