[GitHub] drill issue #1012: DRILL-5911: Upgrade esri-geometry-api version to 2.0.0 to...
Github user julianhyde commented on the issue: https://github.com/apache/drill/pull/1012 If I recall correctly -- we went through this because Calcite uses ESRI -- the only change was to remove org.json. It's a pretty important change because org.json is not compatible with Apache license. And I think org.json data structures were used in the API (e.g. as arguments and method return values) so obsoleting it would be an API change. ---
[GitHub] drill pull request #984: DRILL-5783 Made a unit test for generated Priority ...
Github user julianhyde commented on a diff in the pull request:
https://github.com/apache/drill/pull/984#discussion_r145561518
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/test/ClusterFixture.java ---
@@ -584,11 +492,14 @@ public static void defineWorkspace(Drillbit drillbit,
String pluginName,
public static final String EXPLAIN_PLAN_TEXT = "text";
public static final String EXPLAIN_PLAN_JSON = "json";
- public static FixtureBuilder builder() {
-FixtureBuilder builder = new FixtureBuilder()
+ public static FixtureBuilder builder(DirTestWatcher dirTestWatcher) {
--- End diff --
Jeez Paul, please start a review rather than making single review comments.
I just got 39 emails from you, and so did everyone else on dev@drill.
---
[GitHub] drill issue #685: Drill 5043: Function that returns a unique id per session/...
Github user julianhyde commented on the issue:
https://github.com/apache/drill/pull/685
To be consistent with other the other context functions in standard SQL
({{CURRENT_USER}}, {{SESSION_USER}}, {{CURRENT_PATH}}, {{CURRENT_ROLE}},
{{LOCALTIMESTAMP}}) this should be invoked without parameters: {{session_id}}
rather than {{session_id()}}.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #671: DRILL-4347: Propagate distinct row count for joins from lo...
Github user julianhyde commented on the issue: https://github.com/apache/drill/pull/671 Rebasing onto Calcite is like running after a train: you can't just stop and take a rest. :) And by the way, the state of Drill-Arrow integration makes me very sad. Now Drill has fallen behind there, I doubt whether it will ever catch up. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #671: DRILL-4347: Propagate distinct row count for joins from lo...
Github user julianhyde commented on the issue: https://github.com/apache/drill/pull/671 Yes, CachingRelMetadataProvider is meant for this. After https://issues.apache.org/jira/browse/CALCITE-604, providers are considerably more efficient (not called via reflection), and RelMetadataQuery contains a per-request cache (because some metadata does change, but slowly). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #549: DRILL-4682: Allow full schema identifier in SELECT ...
Github user julianhyde commented on a diff in the pull request:
https://github.com/apache/drill/pull/549#discussion_r72143283
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/DrillCompoundIdentifier.java
---
@@ -69,31 +70,38 @@ public void addIndex(int index, SqlParserPos pos){
}
}
- public SqlNode getAsSqlNode(){
-if(ids.size() == 1){
+ public SqlNode getAsSqlNode(Set fullSchemasSet)
{
--- End diff --
I can't argue with the facts.
But you're writing an ugly piece of code and building up technical debt.
That is a bad idea.
Maybe you need to revisit how you deal with JSON arrays.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #549: DRILL-4682: Allow full schema identifier in SELECT ...
Github user julianhyde commented on a diff in the pull request:
https://github.com/apache/drill/pull/549#discussion_r71924583
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/DrillCompoundIdentifier.java
---
@@ -69,31 +70,38 @@ public void addIndex(int index, SqlParserPos pos){
}
}
- public SqlNode getAsSqlNode(){
-if(ids.size() == 1){
+ public SqlNode getAsSqlNode(Set fullSchemasSet)
{
--- End diff --
Calcite has a concept of a "namespace" that abstracts what columns are
available in a table or sub-query. I think you should be using that rather than
looking at the structure of the parse tree.
There's a lot of code here, and it seems to duplicate (in a less general
way) what is being done in Calcite. It's technical debt, and let me explain how
it will bite Drill. I am working right now on
https://issues.apache.org/jira/browse/CALCITE-1208 and making some significant
changes to how name-resolution works. When I check in CALCITE-1208 there's a
strong chance that the code in this PR will break, and as a result, it will
take Drill even longer to get back onto Calcite master branch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #541: DRILL-4673: Implement "DROP TABLE IF EXISTS" for drill to ...
Github user julianhyde commented on the issue: https://github.com/apache/drill/pull/541 The method in the parser and the class should both be called `DropTable`. `ifExists` should just be a boolean field. At some point you might want to add an `OR REPLACE` clause to `CREATE VIEW`, and add `IF NOT EXISTS` to `CREATE TABLE`. You would not want to rename those classes, either. Just add a boolean field. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #541: DRILL-4673: Implement "DROP TABLE IF EXISTS" for drill to ...
Github user julianhyde commented on the issue: https://github.com/apache/drill/pull/541 Why rename DropTable to DropTableIfExists? It's still the DROP TABLE command. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4514 : Add describe schema ...
Github user julianhyde commented on the pull request: https://github.com/apache/drill/pull/436#issuecomment-200448075 Yes, I think this would be good in Calcite. Thanks for offering. DESCRIBE is in the SQL standard (the latest draft, anyway; I didn't check any others) with a slightly different purpose -- to describe the columns of a query. (It reminds me a bit of MySQL, where DESCRIBE and EXPLAIN are synonyms.) That's good news, because it means we're not adding another reserved word, but we also need to make sure that DESCRIBE TABLE, DESCRIBE SCHEMA, DESCRIBE DATABASE are compatible with the standard syntax. ``` 20.9 This Subclause is modified by Subclause 17.4, ââ, in ISO/IEC 9075-9. Function Obtain information about the columns or s contained in a prepared statement or about the columns of the result set associated with a cursor. Format ::= | ::= DESCRIBE INPUT [ ] ::= DESCRIBE [ OUTPUT ] [ ] ::= WITH NESTING | WITHOUT NESTING ::= USING [ SQL ] DESCRIPTOR ::= | CURSOR STRUCTURE ``` The output for DESCRIBE TABLE should be the same as Jdbc getTables. Do you agree? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3623: Use shorter query path for LIMIT 0...
Github user julianhyde commented on a diff in the pull request:
https://github.com/apache/drill/pull/193#discussion_r45012029
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillDirectScanRel.java
---
@@ -0,0 +1,111 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.planner.logical;
+
+import com.google.common.collect.Iterators;
+import org.apache.calcite.plan.RelOptCluster;
+import org.apache.calcite.plan.RelTraitSet;
+import org.apache.calcite.rel.AbstractRelNode;
+import org.apache.calcite.rel.RelWriter;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.drill.common.logical.data.LogicalOperator;
+import org.apache.drill.exec.physical.base.PhysicalOperator;
+import org.apache.drill.exec.planner.physical.DrillScanPrel;
+import org.apache.drill.exec.planner.physical.PhysicalPlanCreator;
+import org.apache.drill.exec.planner.physical.PlannerSettings;
+import org.apache.drill.exec.planner.physical.Prel;
+import org.apache.drill.exec.planner.physical.PrelUtil;
+import org.apache.drill.exec.planner.physical.visitor.PrelVisitor;
+import org.apache.drill.exec.record.BatchSchema;
+import org.apache.drill.exec.store.direct.DirectGroupScan;
+
+import java.io.IOException;
+import java.util.Iterator;
+
+/**
+ * Logical and physical RelNode representing a {@link DirectGroupScan}.
This is not backed by a {@link DrillTable},
+ * unlike {@link DrillScanRel}.
+ */
+public class DrillDirectScanRel extends AbstractRelNode implements
DrillScanPrel, DrillRel {
--- End diff --
Yeah, I think DrillDirectScanRel is a wrong turn for this functionality,
because you can't see that it is empty and reason about it. A DirectGroupScan
is a runtime thing, so shouldn't be floating around at planning time.
I notice that DrillValuesRel does not extend Values. If it did, it would
get all of the rules in PruneEmptyRules for free - pruning away Filter, Project
etc. on top of it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3623: Use shorter query path for LIMIT 0...
Github user julianhyde commented on a diff in the pull request:
https://github.com/apache/drill/pull/193#discussion_r43316183
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/FindLimit0Visitor.java
---
@@ -46,6 +51,32 @@ public static boolean containsLimit0(RelNode rel) {
return visitor.isContains();
}
+ public static DrillRel addLimitOnTopOfLeafNodes(final DrillRel rel) {
+final RelShuttleImpl shuttle = new RelShuttleImpl() {
+
+ private RelNode addLimitAsParent(RelNode node) {
+final RexBuilder builder = node.getCluster().getRexBuilder();
+final RexLiteral offset =
builder.makeExactLiteral(BigDecimal.ZERO);
+final RexLiteral fetch = builder.makeExactLiteral(BigDecimal.ZERO);
+return new DrillLimitRel(node.getCluster(), node.getTraitSet(),
node, offset, fetch);
--- End diff --
Agree with @jinfengni. In more recent versions of Calcite, use
RelBuilder.limit() or .sortLimit(). The RelBuilder will be configured to create
the appropriate Drill variants of all RelNodes. It might also do some useful
canonization/optimization. We recommend using RelBuilder for most tasks
involving creating RelNodes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...
Github user julianhyde commented on a diff in the pull request:
https://github.com/apache/drill/pull/169#discussion_r40447157
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/LimitUnionExchangeTransposeRule.java
---
@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.planner.physical;
+
+import org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.drill.exec.planner.logical.RelOptHelper;
+
+import java.math.BigDecimal;
+
+public class LimitUnionExchangeTransposeRule extends Prule{
+ public static final RelOptRule INSTANCE = new
LimitUnionExchangeTransposeRule();
+
+ private LimitUnionExchangeTransposeRule() {
+super(RelOptHelper.some(LimitPrel.class,
RelOptHelper.any(UnionExchangePrel.class)), "LimitUnionExchangeTransposeRule");
+ }
+
+ @Override
+ public boolean matches(RelOptRuleCall call) {
+final LimitPrel limit = (LimitPrel) call.rel(0);
+
+return !limit.isPushDown();
+ }
+
+ @Override
+ public void onMatch(RelOptRuleCall call) {
+final LimitPrel limit = (LimitPrel) call.rel(0);
+final UnionExchangePrel unionExchangePrel = (UnionExchangePrel)
call.rel(1);
+
+RelNode child = unionExchangePrel.getInput();
+
+final int offset = limit.getOffset() != null ? Math.max(0,
RexLiteral.intValue(limit.getOffset())) : 0;
+final int fetch = limit.getFetch() != null? Math.max(0,
RexLiteral.intValue(limit.getFetch())) : 0;
--- End diff --
0 is a valid limit. I think you need -1 here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...
Github user julianhyde commented on a diff in the pull request:
https://github.com/apache/drill/pull/169#discussion_r40446899
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillLimitRelBase.java
---
@@ -36,11 +36,17 @@
public abstract class DrillLimitRelBase extends SingleRel implements
DrillRelNode {
protected RexNode offset;
protected RexNode fetch;
+ private boolean pushDown; // whether limit has been push past its child.
--- End diff --
Actually sort and aggregate have some of this behavior too. You can do a
partial sort or partial aggregate in the children but then you need to combine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Github user julianhyde commented on the pull request: https://github.com/apache/drill/pull/159#issuecomment-143310179 Yes, open a JIRA. We don't need NonReservedKeyWord() and CommonNonReservedKeyWord() to be separate anymore, so you can generate code into whichever one is most convenient. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-1457: Push Limit past through UnionExcha...
Github user julianhyde commented on a diff in the pull request:
https://github.com/apache/drill/pull/169#discussion_r40465777
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/LimitUnionExchangeTransposeRule.java
---
@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.planner.physical;
+
+import org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.drill.exec.planner.logical.RelOptHelper;
+
+import java.math.BigDecimal;
+
+public class LimitUnionExchangeTransposeRule extends Prule{
+ public static final RelOptRule INSTANCE = new
LimitUnionExchangeTransposeRule();
+
+ private LimitUnionExchangeTransposeRule() {
+super(RelOptHelper.some(LimitPrel.class,
RelOptHelper.any(UnionExchangePrel.class)), "LimitUnionExchangeTransposeRule");
+ }
+
+ @Override
+ public boolean matches(RelOptRuleCall call) {
+final LimitPrel limit = (LimitPrel) call.rel(0);
+
+return !limit.isPushDown();
+ }
+
+ @Override
+ public void onMatch(RelOptRuleCall call) {
+final LimitPrel limit = (LimitPrel) call.rel(0);
+final UnionExchangePrel unionExchangePrel = (UnionExchangePrel)
call.rel(1);
+
+RelNode child = unionExchangePrel.getInput();
+
+final int offset = limit.getOffset() != null ? Math.max(0,
RexLiteral.intValue(limit.getOffset())) : 0;
+final int fetch = limit.getFetch() != null? Math.max(0,
RexLiteral.intValue(limit.getFetch())) : 0;
--- End diff --
@hsuanyi There are several changes going into Calcite for optimizing
limits. See https://issues.apache.org/jira/browse/CALCITE-831 and re-use those
rules in drill if applicable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
