[GitHub] [drill] vvysotskyi commented on a diff in pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

vvysotskyi commented on code in PR #2599:
URL: https://github.com/apache/drill/pull/2599#discussion_r1083511030


##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillSetOpRel.java:
##
@@ -0,0 +1,93 @@
+/*
+ * 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 org.apache.calcite.linq4j.Ord;
+import org.apache.calcite.plan.RelOptCluster;
+import org.apache.calcite.plan.RelTraitSet;
+import org.apache.calcite.rel.InvalidRelException;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.drill.common.logical.data.LogicalOperator;
+import org.apache.drill.common.logical.data.Union;
+import org.apache.drill.exec.planner.common.DrillSetOpRelBase;
+
+import java.util.List;
+
+/**
+ * SetOp implemented in Drill.
+ */
+public class DrillSetOpRel extends DrillSetOpRelBase implements DrillRel {

Review Comment:
   I think instead of having a common class for the intersect and except, it 
would be better to differ them as it is done in Calcite and extend their 
implementations, so it will help to remove defining custom `estimateRowCount` 
methods and allow using more optimizations designed for these operators, like 
push down to JDBC and so on.



##
exec/java-exec/src/test/java/org/apache/drill/TestSetOp.java:
##
@@ -0,0 +1,1093 @@
+/*
+ * 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;
+
+import org.apache.drill.exec.planner.physical.PlannerSettings;
+import org.apache.drill.exec.record.BatchSchema;
+import org.apache.drill.exec.record.BatchSchemaBuilder;
+import org.apache.drill.exec.record.metadata.SchemaBuilder;
+import org.apache.drill.shaded.guava.com.google.common.collect.Lists;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.drill.categories.OperatorTest;
+import org.apache.drill.categories.SqlTest;
+import org.apache.drill.categories.UnlikelyTest;
+import org.apache.drill.common.exceptions.UserException;
+import org.apache.drill.common.expression.SchemaPath;
+import org.apache.drill.common.types.TypeProtos;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.test.ClusterFixture;
+import org.apache.drill.test.ClusterTest;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.nio.file.Paths;
+import java.util.List;
+
+@Category({SqlTest.class, OperatorTest.class})
+public class TestSetOp extends ClusterTest {

Review Comment:
   It is good that it is handled in the code, but it could also be fine to have 
a test that verifies it works as expected, so we will be sure that no future 
changes will break it.



##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/setop/HashSetOpProbeTemplate.java:
##
@@ -0,0 +1,354 @@
+/*
+ * 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 applicab

[GitHub] [drill] cgivre opened a new pull request, #2744: DRILL-8392: Empty Tables Causes Index Out of Bounds Exception on PDF Reader

[via GitHub]

cgivre opened a new pull request, #2744:
URL: https://github.com/apache/drill/pull/2744

   # [DRILL-8392](https://issues.apache.org/jira/browse/DRILL-): Empty 
Tables Causes Index Out of Bounds Exception on PDF Reader
   
   ## Description
   In certain conditions, Drill will generate an Index Out of Bounds exception. 
 This minor PR fixes that.  Basically, when a schema is not provided, Drill 
will ignore empty cells in rows. 
   
   ## Documentation
   No user facing changes.
   
   ## Testing
   Ran existing unit tests and tested with customer data.


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] LYCJeff commented on issue #2735: Use some configuration items to specify the parameters as filters that allow them to be passed to headers and post body through SQL dynamically

[via GitHub]

LYCJeff commented on issue #2735:
URL: https://github.com/apache/drill/issues/2735#issuecomment-1403221114

   > @LYCJeff Drill already does this. Take a look at the docs 
(https://github.com/apache/drill/tree/master/contrib/storage-http#method) for 
the `postBodyLocation` parameter.
   > 
   > I actually like your design better however.
   
   @cgivre Should I add an improvement on JIRA？


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre commented on issue #2735: Use some configuration items to specify the parameters as filters that allow them to be passed to headers and post body through SQL dynamically

[via GitHub]

cgivre commented on issue #2735:
URL: https://github.com/apache/drill/issues/2735#issuecomment-1403660469

   @LYCJeff That would be great!  We're gearing up for a release, so it would 
probably not be for this one.   Is this something you'd want to contribute?


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre merged pull request #2744: DRILL-8392: Empty Tables Causes Index Out of Bounds Exception on PDF Reader

[via GitHub]

cgivre merged PR #2744:
URL: https://github.com/apache/drill/pull/2744


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] LYCJeff commented on issue #2735: Use some configuration items to specify the parameters as filters that allow them to be passed to headers and post body through SQL dynamically

[via GitHub]

LYCJeff commented on issue #2735:
URL: https://github.com/apache/drill/issues/2735#issuecomment-1403722861

   > @LYCJeff That would be great! We're gearing up for a release, so it would 
probably not be for this one. Is this something you'd want to contribute?
   
   @cgivre Yes, I would like to contribute this to drill, which I really enjoy.


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre commented on issue #2735: Use some configuration items to specify the parameters as filters that allow them to be passed to headers and post body through SQL dynamically

[via GitHub]

cgivre commented on issue #2735:
URL: https://github.com/apache/drill/issues/2735#issuecomment-1403724790

   @LYCJeff Excellent!  I'm happy to help you out.  Drill can be a complicated 
beast to navigate, so please don't hesitate to reach out if you have any 
questions.   My email is cgi...@apache.org.


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] LYCJeff commented on issue #2735: Use some configuration items to specify the parameters as filters that allow them to be passed to headers and post body through SQL dynamically

[via GitHub]

LYCJeff commented on issue #2735:
URL: https://github.com/apache/drill/issues/2735#issuecomment-1403834782

   > @LYCJeff Excellent! I'm happy to help you out. Drill can be a complicated 
beast to navigate, so please don't hesitate to reach out if you have any 
questions. My email is [cgi...@apache.org](mailto:cgi...@apache.org).
   
   @cgivre Thx for your help, I'm glad to have the opportunity to learn from 
you. 
   I added an improvement on JIRA, could you please see if there is a problem? 
(DRILL-8393,   
https://issues.apache.org/jira/browse/DRILL-8393?jql=project%20%3D%20DRILL)


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] paul-rogers commented on a diff in pull request #2728: DRILL-8372: Unfreed buffers when running a LIMIT 0 query over delimited text

[via GitHub]

paul-rogers commented on code in PR #2728:
URL: https://github.com/apache/drill/pull/2728#discussion_r1087483902


##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/limit/LimitRecordBatch.java:
##
@@ -75,7 +75,7 @@ public IterOutcome innerNext() {
 upStream = next(incoming);
   }
   // If EMIT that means leaf operator is UNNEST, in this case refresh the 
limit states and return EMIT.
-  if (upStream == EMIT) {
+  if (upStream == EMIT || upStream == NONE) {

Review Comment:
   This doesn't seem to be quite the right solution. This block of code is for 
a very particular case: an UNNEST.
   
   Expanding this code, look at the top of the loop:
   
   ```java
   if (!first && !needMoreRecords(numberOfRecords)) {
   ```
   
   With a LIMIT 0, we hit the limit on the first batch. I'm not quite sure why 
the `!first` is in place. Maybe history would tell us. Perhaps the right answer 
is something like:
   
   ```java
   if ( !needMoreRecords(numberOfRecords)) {
outgoingSv.setRecordCount(0);
VectorAccessibleUtilities.clear(incoming);
return super.innerNext();
   }
   if (!first) {
 ...
   ```
   
   I suspect that the logic actually needs more analysis. What does it do on 
the first batch now? What does `super.innerNext()` do, and do we want that if 
we've reached the limit?
   
   Generally, the debugger is the best way to sort this out. Try a LIMIT 0, a 
LIMIT n where n < size of the first batch, LIMIT n where n > batch size && n < 
2 * batch size, 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.

To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] Leon-WTF commented on a diff in pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

Leon-WTF commented on code in PR #2599:
URL: https://github.com/apache/drill/pull/2599#discussion_r1088599209


##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/setop/HashSetOpProbeTemplate.java:
##
@@ -0,0 +1,354 @@
+/*
+ * 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.physical.impl.setop;
+
+import org.apache.calcite.sql.SqlKind;
+import org.apache.drill.exec.exception.SchemaChangeException;
+import org.apache.drill.exec.physical.impl.common.HashPartition;
+import org.apache.drill.exec.physical.impl.join.HashJoinHelper;
+import org.apache.drill.exec.record.BatchSchema;
+import org.apache.drill.exec.record.RecordBatch;
+import org.apache.drill.exec.record.RecordBatch.IterOutcome;
+import org.apache.drill.exec.record.VectorContainer;
+import org.apache.drill.exec.record.VectorWrapper;
+import org.apache.drill.exec.vector.IntVector;
+import org.apache.drill.exec.vector.ValueVector;
+
+import java.util.ArrayList;
+
+import static org.apache.drill.exec.record.JoinBatchMemoryManager.LEFT_INDEX;
+
+public class HashSetOpProbeTemplate implements HashSetOpProbe {

Review Comment:
   It's more like HashJoinProbeTemplate, I have refactored it.



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] Leon-WTF commented on a diff in pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

Leon-WTF commented on code in PR #2599:
URL: https://github.com/apache/drill/pull/2599#discussion_r1089851904


##
exec/java-exec/src/test/java/org/apache/drill/TestSetOp.java:
##
@@ -0,0 +1,1093 @@
+/*
+ * 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;
+
+import org.apache.drill.exec.planner.physical.PlannerSettings;
+import org.apache.drill.exec.record.BatchSchema;
+import org.apache.drill.exec.record.BatchSchemaBuilder;
+import org.apache.drill.exec.record.metadata.SchemaBuilder;
+import org.apache.drill.shaded.guava.com.google.common.collect.Lists;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.drill.categories.OperatorTest;
+import org.apache.drill.categories.SqlTest;
+import org.apache.drill.categories.UnlikelyTest;
+import org.apache.drill.common.exceptions.UserException;
+import org.apache.drill.common.expression.SchemaPath;
+import org.apache.drill.common.types.TypeProtos;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.test.ClusterFixture;
+import org.apache.drill.test.ClusterTest;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.nio.file.Paths;
+import java.util.List;
+
+@Category({SqlTest.class, OperatorTest.class})
+public class TestSetOp extends ClusterTest {

Review Comment:
   Sure, I have added 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.

To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton opened a new pull request, #2745: DRILL-8394: ANALYZE TABLE ... COMPUTE STATISTICS fails with a trailing slash

[via GitHub]

jnturton opened a new pull request, #2745:
URL: https://github.com/apache/drill/pull/2745

   # [DRILL-8394](https://issues.apache.org/jira/browse/DRILL-8394): ANALYZE 
TABLE ... COMPUTE STATISTICS fails with a trailing slash
   
   ## Description
   
   1. Add a method to extract the table name from a SqlIdentifier, trimming 
trailing slashes in the process.
   2. Add a unit test.
   3. Fix the spelling error in the class name 
org.apache.drill.exec.planner.sql.SchemaUtilites.
   
   ## Documentation
   N/A
   
   ## Testing
   New unit 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.

To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton merged pull request #2745: DRILL-8394: ANALYZE TABLE ... COMPUTE STATISTICS fails with a trailing slash

[via GitHub]

jnturton merged PR #2745:
URL: https://github.com/apache/drill/pull/2745


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] Z0ltrix opened a new issue, #2746: [DISCUSSION] Use INT96 as default timestamp format in Parquet tables

[via GitHub]

Z0ltrix opened a new issue, #2746:
URL: https://github.com/apache/drill/issues/2746

   Hi everyone,
   
   i want to raise a discussion about the current behavior in drill regarding 
parquet timestamps. 
   
   Drill uses `INT64` for timestamps and you can switch to `INT96` by setting 
`store.parquet.reader.int96_as_timestamp` to `true`. With that its not a big 
problem to work with both types of parquet timestamps, but since that spark 
uses `INT96` as default, you have to switch this configure in almost all 
situations, especially when working with new lakehouse architectures like 
deltalake and iceberg.
   
   For spark its clearly documented that they use INT96 in all scenarios:
   
   here  for reading -> 
https://spark.apache.org/docs/latest/sql-data-sources-parquet.html 
   
   >  Some Parquet-producing systems, in particular Impala and Hive, store 
Timestamp into INT96. This flag tells Spark SQL to interpret INT96 data as a 
timestamp to provide compatibility with these systems. 
   
   here for writing-> https://spark.apache.org/docs/latest/configuration.html
   
   > Sets which Parquet timestamp type to use when Spark writes data to Parquet 
files. INT96 is a non-standard but commonly used timestamp type in Parquet. 
TIMESTAMP_MICROS is a standard timestamp type in Parquet, which stores number 
of microseconds from the Unix epoch. TIMESTAMP_MILLIS is also standard, but 
with millisecond precision, which means Spark has to truncate the microsecond 
portion of its timestamp value.
   
   Of course we could advise every drill user to write its spark jobs with the 
configuration `spark.sql.parquet.outputTimestampType` to `TIMESTAMP_MICROS` or 
`TIMESTAMP_MILLIS` or always toggle this drill configuration after startup, but 
its still an additional step. 
   
   @vvysotskyi  mentioned that if we would switch this default now, we would 
have issues with some UDF´s, so i would think it could be a topic for upcomming 
Drill 2.0.0 as a breaking change.
   
   What do you think?


-- 
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: dev-unsubscr...@drill.apache.org.apache.org

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

[GitHub] [drill] cgivre commented on issue #2746: [DISCUSSION] Use INT96 as default timestamp format in Parquet tables

[via GitHub]

cgivre commented on issue #2746:
URL: https://github.com/apache/drill/issues/2746#issuecomment-1412101491

   I'll weigh in here.  It seems that since this is user configurable, it would 
make sense to make that the default and fix the UDFs.  We're about to release 
1.21 which has a lot of major improvements, so IMHO it would be a good time to 
do so.
   
   Vova, would you mind explaining how this will break UDFs?
   Best,
   -- C
   
   
   
   > On Feb 1, 2023, at 7:54 AM, Christian Pfarr ***@***.***> wrote:
   > 
   > 
   > Hi everyone,
   > 
   > i want to raise a discussion about the current behavior in drill regarding 
parquet timestamps.
   > 
   > Drill uses INT64 for timestamps and you can switch to INT96 by setting 
store.parquet.reader.int96_as_timestamp to true. With that its not a big 
problem to work with both types of parquet timestamps, but since that spark 
uses INT96 as default, you have to switch this configure in almost all 
situations, especially when working with new lakehouse architectures like 
deltalake and iceberg.
   > 
   > For spark its clearly documented that they use INT96 in all scenarios:
   > 
   > here for reading -> 
https://spark.apache.org/docs/latest/sql-data-sources-parquet.html
   > 
   > Some Parquet-producing systems, in particular Impala and Hive, store 
Timestamp into INT96. This flag tells Spark SQL to interpret INT96 data as a 
timestamp to provide compatibility with these systems.
   > 
   > here for writing-> https://spark.apache.org/docs/latest/configuration.html
   > 
   > Sets which Parquet timestamp type to use when Spark writes data to Parquet 
files. INT96 is a non-standard but commonly used timestamp type in Parquet. 
TIMESTAMP_MICROS is a standard timestamp type in Parquet, which stores number 
of microseconds from the Unix epoch. TIMESTAMP_MILLIS is also standard, but 
with millisecond precision, which means Spark has to truncate the microsecond 
portion of its timestamp value.
   > 
   > Of course we could advise every drill user to write its spark jobs with 
the configuration spark.sql.parquet.outputTimestampType to TIMESTAMP_MICROS or 
TIMESTAMP_MILLIS or always toggle this drill configuration after startup, but 
its still an additional step.
   > 
   > @vvysotskyi  mentioned that if we would 
switch this default now, we would have issues with some UDF´s, so i would think 
it could be a topic for upcomming Drill 2.0.0 as a breaking change.
   > 
   > What do you think?
   > 
   > —
   > Reply to this email directly, view it on GitHub 
, or unsubscribe 
.
   > You are receiving this because you are subscribed to this thread.
   > 
   
   


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton commented on a diff in pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

jnturton commented on code in PR #2499:
URL: https://github.com/apache/drill/pull/2499#discussion_r1093275447


##
exec/java-exec/src/test/java/org/apache/drill/test/ClusterTest.java:
##
@@ -125,4 +132,56 @@ public static void run(String query, Object... args) 
throws Exception {
   public QueryBuilder queryBuilder( ) {
 return client.queryBuilder();
   }
+
+  /**
+   * Utility method which tests given query produces a {@link UserException} 
and the exception message contains
+   * the given message.
+   * @param testSqlQuery Test query
+   * @param expectedErrorMsg Expected error message.
+   */
+  protected static void errorMsgTestHelper(String testSqlQuery, String 
expectedErrorMsg) throws Exception {
+try {
+  run(testSqlQuery);
+  fail("Expected a UserException when running " + testSqlQuery);
+} catch (UserException actualException) {
+  try {
+assertThat("message of UserException when running " + testSqlQuery, 
actualException.getMessage(), containsString(expectedErrorMsg));
+  } catch (AssertionError e) {
+e.addSuppressed(actualException);
+throw e;
+  }
+}
+  }
+
+  protected static void updateClient(Properties properties) {
+if (client != null) {
+  client.close();
+  client = null;
+}
+ClientFixture.ClientBuilder clientBuilder = cluster.clientBuilder();
+if (properties != null) {
+  for (final String key : properties.stringPropertyNames()) {
+final String lowerCaseKey = key.toLowerCase();
+clientBuilder.property(lowerCaseKey, properties.getProperty(key));
+  }
+}
+client = clientBuilder.build();
+  }
+
+  protected static void updateClient(final String user) {
+updateClient(user, null);
+  }
+
+  protected static void updateClient(final String user, final String password) 
{
+if (client != null) {
+  client.close();
+  client = null;
+}
+final Properties properties = new Properties();
+properties.setProperty(DrillProperties.USER, user);
+if (password != null) {
+  properties.setProperty(DrillProperties.PASSWORD, password);
+}
+updateClient(properties);
+  }

Review Comment:
   Let's move these additions to ClientFixture. It will mean that they're no 
longer drop-in replacements but they'll be more at home 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.

To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton commented on issue #2746: [DISCUSSION] Use INT96 as default timestamp format in Parquet tables

[via GitHub]

jnturton commented on issue #2746:
URL: https://github.com/apache/drill/issues/2746#issuecomment-1412173964

   I'm a -1 at this point. Even though it has historically been a popular thing 
to do, the use of INT96 for timestamps is nonstandard and deprecated [1]. The 
Parquet project wants to discourage its ongoing use [2].
   
   1. https://github.com/apache/parquet-format/pull/86/files
   2. https://issues.apache.org/jira/browse/PARQUET-323
   
   So I think we should leave this switched off by default and change the 
description of our option to say that INT96 timestamps are deprecated and we 
only recommend enabling them for the sake of reading legacy data.


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] Z0ltrix commented on issue #2746: [DISCUSSION] Use INT96 as default timestamp format in Parquet tables

[via GitHub]

Z0ltrix commented on issue #2746:
URL: https://github.com/apache/drill/issues/2746#issuecomment-1412186895

   @jnturton I am of your opinion. Wasn't aware of this situation.
   
   I wonder why spark has this still as a default config.


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton commented on issue #2746: [DISCUSSION] Use INT96 as default timestamp format in Parquet tables

[via GitHub]

jnturton commented on issue #2746:
URL: https://github.com/apache/drill/issues/2746#issuecomment-1412199513

   @Z0ltrix it looks like they were worried about compatibility with Hive and 
Presto when they last attempted to change.
   
   https://github.com/apache/spark/pull/28450
   
   I guess because Spark is used much more for ETL and Parquet _writing_ they 
have to be more conservative about the compatibility of their outputs.


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] vvysotskyi commented on issue #2746: [DISCUSSION] Use INT96 as default timestamp format in Parquet tables

[via GitHub]

vvysotskyi commented on issue #2746:
URL: https://github.com/apache/drill/issues/2746#issuecomment-1412711824

   The UDF that doesn't work when this option is enabled is 
`CONVERT_FROM(timestamp_field, 'TIMESTAMP_IMPALA')`, and it doesn't work 
because the `timestamp_field` type becomes timestamp instead of binary, and it 
is expected behavior, no need to fix it. Details regarding this option can be 
found here: https://drill.apache.org/docs/parquet-format/#about-int96-support
   
   Side note: `INT96` parquet physical type is deprecated, not only the usage 
for timestamps.
   
   There are a lot of cases when such data could be passed to Drill. The good 
news is that regardless of the option value, Drill doesn't write this type, it 
only reads it and interprets data as timestamp values instead of binary when 
the option is enabled. 
   So I think that enabling the option (and possibly keeping behavior in sync 
with Spark and other tools) will only improve the user experience.


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton commented on issue #2746: [DISCUSSION] Use INT96 as default timestamp format in Parquet tables

[via GitHub]

jnturton commented on issue #2746:
URL: https://github.com/apache/drill/issues/2746#issuecomment-1413111702

   I guess the only loss of enabling it by default, apart from not helping 
Parquet to push people away from it, would be worse UX for people who were 
using INT96 as a byte array or a very wide integer. And that's probably no one 
at all.
   
   I'll update my vote to a +0.


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] kingswanwho commented on a diff in pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

kingswanwho commented on code in PR #2499:
URL: https://github.com/apache/drill/pull/2499#discussion_r1094205201


##
exec/java-exec/src/test/java/org/apache/drill/test/ClusterTest.java:
##
@@ -125,4 +132,56 @@ public static void run(String query, Object... args) 
throws Exception {
   public QueryBuilder queryBuilder( ) {
 return client.queryBuilder();
   }
+
+  /**
+   * Utility method which tests given query produces a {@link UserException} 
and the exception message contains
+   * the given message.
+   * @param testSqlQuery Test query
+   * @param expectedErrorMsg Expected error message.
+   */
+  protected static void errorMsgTestHelper(String testSqlQuery, String 
expectedErrorMsg) throws Exception {
+try {
+  run(testSqlQuery);
+  fail("Expected a UserException when running " + testSqlQuery);
+} catch (UserException actualException) {
+  try {
+assertThat("message of UserException when running " + testSqlQuery, 
actualException.getMessage(), containsString(expectedErrorMsg));
+  } catch (AssertionError e) {
+e.addSuppressed(actualException);
+throw e;
+  }
+}
+  }
+
+  protected static void updateClient(Properties properties) {
+if (client != null) {
+  client.close();
+  client = null;
+}
+ClientFixture.ClientBuilder clientBuilder = cluster.clientBuilder();
+if (properties != null) {
+  for (final String key : properties.stringPropertyNames()) {
+final String lowerCaseKey = key.toLowerCase();
+clientBuilder.property(lowerCaseKey, properties.getProperty(key));
+  }
+}
+client = clientBuilder.build();
+  }
+
+  protected static void updateClient(final String user) {
+updateClient(user, null);
+  }
+
+  protected static void updateClient(final String user, final String password) 
{
+if (client != null) {
+  client.close();
+  client = null;
+}
+final Properties properties = new Properties();
+properties.setProperty(DrillProperties.USER, user);
+if (password != null) {
+  properties.setProperty(DrillProperties.PASSWORD, password);
+}
+updateClient(properties);
+  }

Review Comment:
   HI James, ClientFixture doesn't have client variable, so that we could't 
close client at first. And cluster here is a non-static variable, we should 
change all updateClient static method to non-static method, should we still 
keep changing this?



##
exec/java-exec/src/test/java/org/apache/drill/test/ClusterTest.java:
##
@@ -125,4 +132,56 @@ public static void run(String query, Object... args) 
throws Exception {
   public QueryBuilder queryBuilder( ) {
 return client.queryBuilder();
   }
+
+  /**
+   * Utility method which tests given query produces a {@link UserException} 
and the exception message contains
+   * the given message.
+   * @param testSqlQuery Test query
+   * @param expectedErrorMsg Expected error message.
+   */
+  protected static void errorMsgTestHelper(String testSqlQuery, String 
expectedErrorMsg) throws Exception {
+try {
+  run(testSqlQuery);
+  fail("Expected a UserException when running " + testSqlQuery);
+} catch (UserException actualException) {
+  try {
+assertThat("message of UserException when running " + testSqlQuery, 
actualException.getMessage(), containsString(expectedErrorMsg));
+  } catch (AssertionError e) {
+e.addSuppressed(actualException);
+throw e;
+  }
+}
+  }
+
+  protected static void updateClient(Properties properties) {
+if (client != null) {
+  client.close();
+  client = null;
+}
+ClientFixture.ClientBuilder clientBuilder = cluster.clientBuilder();
+if (properties != null) {
+  for (final String key : properties.stringPropertyNames()) {
+final String lowerCaseKey = key.toLowerCase();
+clientBuilder.property(lowerCaseKey, properties.getProperty(key));
+  }
+}
+client = clientBuilder.build();
+  }
+
+  protected static void updateClient(final String user) {
+updateClient(user, null);
+  }
+
+  protected static void updateClient(final String user, final String password) 
{
+if (client != null) {
+  client.close();
+  client = null;
+}
+final Properties properties = new Properties();
+properties.setProperty(DrillProperties.USER, user);
+if (password != null) {
+  properties.setProperty(DrillProperties.PASSWORD, password);
+}
+updateClient(properties);
+  }

Review Comment:
   Hi James, ClientFixture doesn't have client variable, so that we could't 
close client at first. And cluster here is a non-static variable, we should 
change all updateClient static method to non-static method, should we still 
keep changing 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.

To unsubscribe, e-mail: dev-unsubscr...@d

[GitHub] [drill] kingswanwho commented on a diff in pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

kingswanwho commented on code in PR #2499:
URL: https://github.com/apache/drill/pull/2499#discussion_r1094238769


##
exec/java-exec/src/test/java/org/apache/drill/test/ClusterTest.java:
##
@@ -125,4 +132,56 @@ public static void run(String query, Object... args) 
throws Exception {
   public QueryBuilder queryBuilder( ) {
 return client.queryBuilder();
   }
+
+  /**
+   * Utility method which tests given query produces a {@link UserException} 
and the exception message contains
+   * the given message.
+   * @param testSqlQuery Test query
+   * @param expectedErrorMsg Expected error message.
+   */
+  protected static void errorMsgTestHelper(String testSqlQuery, String 
expectedErrorMsg) throws Exception {
+try {
+  run(testSqlQuery);
+  fail("Expected a UserException when running " + testSqlQuery);
+} catch (UserException actualException) {
+  try {
+assertThat("message of UserException when running " + testSqlQuery, 
actualException.getMessage(), containsString(expectedErrorMsg));
+  } catch (AssertionError e) {
+e.addSuppressed(actualException);
+throw e;
+  }
+}
+  }
+
+  protected static void updateClient(Properties properties) {
+if (client != null) {
+  client.close();
+  client = null;
+}
+ClientFixture.ClientBuilder clientBuilder = cluster.clientBuilder();
+if (properties != null) {
+  for (final String key : properties.stringPropertyNames()) {
+final String lowerCaseKey = key.toLowerCase();
+clientBuilder.property(lowerCaseKey, properties.getProperty(key));
+  }
+}
+client = clientBuilder.build();
+  }
+
+  protected static void updateClient(final String user) {
+updateClient(user, null);
+  }
+
+  protected static void updateClient(final String user, final String password) 
{
+if (client != null) {
+  client.close();
+  client = null;
+}
+final Properties properties = new Properties();
+properties.setProperty(DrillProperties.USER, user);
+if (password != null) {
+  properties.setProperty(DrillProperties.PASSWORD, password);
+}
+updateClient(properties);
+  }

Review Comment:
   Hi James, ClientFixture doesn't have a client variable there, so that we 
could't close client at first and we need to change updateClient signature from 
void to return ClientFixture, besides due to cluster variable here is none 
static, we also need to change updateClient from static method to none static 
method. Furthermore, there are still lots of test cases that we could just 
update client without restart cluster, and for those cases that do need restart 
cluster, we could use startCluster method to config new cluster and client at 
same time without using updateClient. So maybe we could still keep those code 
here?



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] kingswanwho commented on a diff in pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

kingswanwho commented on code in PR #2499:
URL: https://github.com/apache/drill/pull/2499#discussion_r1094240171


##
exec/java-exec/src/test/java/org/apache/drill/test/ClusterTest.java:
##
@@ -125,4 +132,56 @@ public static void run(String query, Object... args) 
throws Exception {
   public QueryBuilder queryBuilder( ) {
 return client.queryBuilder();
   }
+
+  /**
+   * Utility method which tests given query produces a {@link UserException} 
and the exception message contains
+   * the given message.
+   * @param testSqlQuery Test query
+   * @param expectedErrorMsg Expected error message.
+   */
+  protected static void errorMsgTestHelper(String testSqlQuery, String 
expectedErrorMsg) throws Exception {
+try {
+  run(testSqlQuery);
+  fail("Expected a UserException when running " + testSqlQuery);
+} catch (UserException actualException) {
+  try {
+assertThat("message of UserException when running " + testSqlQuery, 
actualException.getMessage(), containsString(expectedErrorMsg));
+  } catch (AssertionError e) {
+e.addSuppressed(actualException);
+throw e;
+  }
+}
+  }
+
+  protected static void updateClient(Properties properties) {
+if (client != null) {
+  client.close();
+  client = null;
+}
+ClientFixture.ClientBuilder clientBuilder = cluster.clientBuilder();
+if (properties != null) {
+  for (final String key : properties.stringPropertyNames()) {
+final String lowerCaseKey = key.toLowerCase();
+clientBuilder.property(lowerCaseKey, properties.getProperty(key));
+  }
+}
+client = clientBuilder.build();
+  }
+
+  protected static void updateClient(final String user) {
+updateClient(user, null);
+  }
+
+  protected static void updateClient(final String user, final String password) 
{
+if (client != null) {
+  client.close();
+  client = null;
+}
+final Properties properties = new Properties();
+properties.setProperty(DrillProperties.USER, user);
+if (password != null) {
+  properties.setProperty(DrillProperties.PASSWORD, password);
+}
+updateClient(properties);
+  }

Review Comment:
   > Let's move these additions to ClientFixture. It will mean that they're no 
longer drop-in replacements but they'll be more at home there.
   
   Hi James, ClientFixture doesn't have a client variable there, so that we 
could't close client at first and we need to change updateClient signature from 
void to return ClientFixture, besides due to cluster variable here is none 
static, we also need to change updateClient from static method to none static 
method. Furthermore, there are still lots of test cases that we could just 
update client without restart cluster, and for those cases that do need restart 
cluster, we could use startCluster method to config new cluster and client at 
same time without using updateClient. So maybe we could still keep those code 
here?



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] kingswanwho commented on a diff in pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

kingswanwho commented on code in PR #2499:
URL: https://github.com/apache/drill/pull/2499#discussion_r1094240724


##
exec/java-exec/src/test/java/org/apache/drill/test/ClusterTest.java:
##
@@ -125,4 +132,56 @@ public static void run(String query, Object... args) 
throws Exception {
   public QueryBuilder queryBuilder( ) {
 return client.queryBuilder();
   }
+
+  /**
+   * Utility method which tests given query produces a {@link UserException} 
and the exception message contains
+   * the given message.
+   * @param testSqlQuery Test query
+   * @param expectedErrorMsg Expected error message.
+   */
+  protected static void errorMsgTestHelper(String testSqlQuery, String 
expectedErrorMsg) throws Exception {
+try {
+  run(testSqlQuery);
+  fail("Expected a UserException when running " + testSqlQuery);
+} catch (UserException actualException) {
+  try {
+assertThat("message of UserException when running " + testSqlQuery, 
actualException.getMessage(), containsString(expectedErrorMsg));
+  } catch (AssertionError e) {
+e.addSuppressed(actualException);
+throw e;
+  }
+}
+  }
+
+  protected static void updateClient(Properties properties) {
+if (client != null) {
+  client.close();
+  client = null;
+}
+ClientFixture.ClientBuilder clientBuilder = cluster.clientBuilder();
+if (properties != null) {
+  for (final String key : properties.stringPropertyNames()) {
+final String lowerCaseKey = key.toLowerCase();
+clientBuilder.property(lowerCaseKey, properties.getProperty(key));
+  }
+}
+client = clientBuilder.build();
+  }
+
+  protected static void updateClient(final String user) {
+updateClient(user, null);
+  }
+
+  protected static void updateClient(final String user, final String password) 
{
+if (client != null) {
+  client.close();
+  client = null;
+}
+final Properties properties = new Properties();
+properties.setProperty(DrillProperties.USER, user);
+if (password != null) {
+  properties.setProperty(DrillProperties.PASSWORD, password);
+}
+updateClient(properties);
+  }

Review Comment:
   Hi James, ClientFixture doesn't have a client variable there, so that we 
could't close client at first and we need to change updateClient signature from 
void to return ClientFixture, besides due to cluster variable here is none 
static, we also need to change updateClient from static method to none static 
method. Furthermore, there are still lots of test cases that we could just 
update client without restart cluster, and for those cases that do need restart 
cluster, we could use startCluster method to config new cluster and client at 
same time without using updateClient. So maybe we could still keep those code 
here?



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] kingswanwho commented on a diff in pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

kingswanwho commented on code in PR #2499:
URL: https://github.com/apache/drill/pull/2499#discussion_r1094240724


##
exec/java-exec/src/test/java/org/apache/drill/test/ClusterTest.java:
##
@@ -125,4 +132,56 @@ public static void run(String query, Object... args) 
throws Exception {
   public QueryBuilder queryBuilder( ) {
 return client.queryBuilder();
   }
+
+  /**
+   * Utility method which tests given query produces a {@link UserException} 
and the exception message contains
+   * the given message.
+   * @param testSqlQuery Test query
+   * @param expectedErrorMsg Expected error message.
+   */
+  protected static void errorMsgTestHelper(String testSqlQuery, String 
expectedErrorMsg) throws Exception {
+try {
+  run(testSqlQuery);
+  fail("Expected a UserException when running " + testSqlQuery);
+} catch (UserException actualException) {
+  try {
+assertThat("message of UserException when running " + testSqlQuery, 
actualException.getMessage(), containsString(expectedErrorMsg));
+  } catch (AssertionError e) {
+e.addSuppressed(actualException);
+throw e;
+  }
+}
+  }
+
+  protected static void updateClient(Properties properties) {
+if (client != null) {
+  client.close();
+  client = null;
+}
+ClientFixture.ClientBuilder clientBuilder = cluster.clientBuilder();
+if (properties != null) {
+  for (final String key : properties.stringPropertyNames()) {
+final String lowerCaseKey = key.toLowerCase();
+clientBuilder.property(lowerCaseKey, properties.getProperty(key));
+  }
+}
+client = clientBuilder.build();
+  }
+
+  protected static void updateClient(final String user) {
+updateClient(user, null);
+  }
+
+  protected static void updateClient(final String user, final String password) 
{
+if (client != null) {
+  client.close();
+  client = null;
+}
+final Properties properties = new Properties();
+properties.setProperty(DrillProperties.USER, user);
+if (password != null) {
+  properties.setProperty(DrillProperties.PASSWORD, password);
+}
+updateClient(properties);
+  }

Review Comment:
   Hi James, ClientFixture doesn't have a client variable there, so that we 
could't close client at first and we need to change updateClient signature from 
void to return ClientFixture, besides due to cluster variable here is none 
static, we also need to change updateClient from static method to none static 
method, and those changes involve lots of code change. Furthermore, there are 
still lots of test cases that we could just update client without restart 
cluster, and for those cases that do need restart cluster, we could use 
startCluster method to config new cluster and client at same time without using 
updateClient. So maybe we could still keep those code here?



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] kingswanwho commented on a diff in pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

kingswanwho commented on code in PR #2499:
URL: https://github.com/apache/drill/pull/2499#discussion_r1094240724


##
exec/java-exec/src/test/java/org/apache/drill/test/ClusterTest.java:
##
@@ -125,4 +132,56 @@ public static void run(String query, Object... args) 
throws Exception {
   public QueryBuilder queryBuilder( ) {
 return client.queryBuilder();
   }
+
+  /**
+   * Utility method which tests given query produces a {@link UserException} 
and the exception message contains
+   * the given message.
+   * @param testSqlQuery Test query
+   * @param expectedErrorMsg Expected error message.
+   */
+  protected static void errorMsgTestHelper(String testSqlQuery, String 
expectedErrorMsg) throws Exception {
+try {
+  run(testSqlQuery);
+  fail("Expected a UserException when running " + testSqlQuery);
+} catch (UserException actualException) {
+  try {
+assertThat("message of UserException when running " + testSqlQuery, 
actualException.getMessage(), containsString(expectedErrorMsg));
+  } catch (AssertionError e) {
+e.addSuppressed(actualException);
+throw e;
+  }
+}
+  }
+
+  protected static void updateClient(Properties properties) {
+if (client != null) {
+  client.close();
+  client = null;
+}
+ClientFixture.ClientBuilder clientBuilder = cluster.clientBuilder();
+if (properties != null) {
+  for (final String key : properties.stringPropertyNames()) {
+final String lowerCaseKey = key.toLowerCase();
+clientBuilder.property(lowerCaseKey, properties.getProperty(key));
+  }
+}
+client = clientBuilder.build();
+  }
+
+  protected static void updateClient(final String user) {
+updateClient(user, null);
+  }
+
+  protected static void updateClient(final String user, final String password) 
{
+if (client != null) {
+  client.close();
+  client = null;
+}
+final Properties properties = new Properties();
+properties.setProperty(DrillProperties.USER, user);
+if (password != null) {
+  properties.setProperty(DrillProperties.PASSWORD, password);
+}
+updateClient(properties);
+  }

Review Comment:
   Hi James, ClientFixture doesn't have a client variable there, so that we 
could't close client at first and we need to change updateClient signature from 
void to return ClientFixture, besides due to cluster variable here is none 
static, we also need to change updateClient from static method to none static 
method, and those modifications involve lots of code change. Furthermore, there 
are still lots of test cases that we could just update client without restart 
cluster, and for those cases that do need restart cluster, we could use 
startCluster method to config new cluster and client at same time without using 
updateClient. So maybe we could still keep those code here?



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton commented on a diff in pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

jnturton commented on code in PR #2499:
URL: https://github.com/apache/drill/pull/2499#discussion_r1094453622


##
exec/java-exec/src/test/java/org/apache/drill/test/ClusterTest.java:
##
@@ -125,4 +132,56 @@ public static void run(String query, Object... args) 
throws Exception {
   public QueryBuilder queryBuilder( ) {
 return client.queryBuilder();
   }
+
+  /**
+   * Utility method which tests given query produces a {@link UserException} 
and the exception message contains
+   * the given message.
+   * @param testSqlQuery Test query
+   * @param expectedErrorMsg Expected error message.
+   */
+  protected static void errorMsgTestHelper(String testSqlQuery, String 
expectedErrorMsg) throws Exception {
+try {
+  run(testSqlQuery);
+  fail("Expected a UserException when running " + testSqlQuery);
+} catch (UserException actualException) {
+  try {
+assertThat("message of UserException when running " + testSqlQuery, 
actualException.getMessage(), containsString(expectedErrorMsg));
+  } catch (AssertionError e) {
+e.addSuppressed(actualException);
+throw e;
+  }
+}
+  }
+
+  protected static void updateClient(Properties properties) {
+if (client != null) {
+  client.close();
+  client = null;
+}
+ClientFixture.ClientBuilder clientBuilder = cluster.clientBuilder();
+if (properties != null) {
+  for (final String key : properties.stringPropertyNames()) {
+final String lowerCaseKey = key.toLowerCase();
+clientBuilder.property(lowerCaseKey, properties.getProperty(key));
+  }
+}
+client = clientBuilder.build();
+  }
+
+  protected static void updateClient(final String user) {
+updateClient(user, null);
+  }
+
+  protected static void updateClient(final String user, final String password) 
{
+if (client != null) {
+  client.close();
+  client = null;
+}
+final Properties properties = new Properties();
+properties.setProperty(DrillProperties.USER, user);
+if (password != null) {
+  properties.setProperty(DrillProperties.PASSWORD, password);
+}
+updateClient(properties);
+  }

Review Comment:
   Ah, yes you're right. These methods still look a bit out of place. Can 
errorMsgTestHelper move to ClientFixture? And can the new updateClient methods 
be replaced by usage of cluster.addClientFixture(Properties p) and 
cluster.client(int number)?



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] LYCJeff opened a new pull request, #2747: DRILL-8393: Allow parameters to be passed to headers through SQL in WHERE clause

[via GitHub]

LYCJeff opened a new pull request, #2747:
URL: https://github.com/apache/drill/pull/2747

   # [DRILL-8393](https://issues.apache.org/jira/browse/DRILL-8393): Allow 
parameters to be passed to headers through SQL in WHERE clause
   
   ## Description
   
   Allow parameters to be passed to headers through SQL in WHERE clause. Use 
the _params_ configuration item to control what parameters are allowed in, 
passing them into the body and header depending on the prefix.
   
   Config:
   `{ "url": "https://api.sunrise-sunset.org/json";, "requireTail": false, 
"params": ["body.lat", "body.lng", "body.date", "header.header1"], 
"parameterLocation": "json_body" }`

   SQL Query:
   ```
   SELECT * FROM api.sunrise
   WHERE `body.lat` = 36.7201600
   AND `body.lng` = -4.4203400
   AND `body.date` = '2019-10-02'
   AND `header.header1` = 'value1';
   ```

   Post body:
   `{ "lat": 36.7201600, "lng": -4.4203400, "date": "2019-10-02"}`

   Headers:
   `{ "header1": "value1", ……}`
   
   ## Documentation
   The _params_ configuration mode and SQL parameter passing mode need to be 
described in the document.
   
   ## Testing
   The _params_ configuration and SQL in the original unit test have been 
changed and have passed this part of the unit 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.

To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] Leon-WTF commented on pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

Leon-WTF commented on PR #2599:
URL: https://github.com/apache/drill/pull/2599#issuecomment-1415544424

   @vvysotskyi Hi, any more comments on 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.

To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton commented on a diff in pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

jnturton commented on code in PR #2499:
URL: https://github.com/apache/drill/pull/2499#discussion_r1095876337


##
exec/java-exec/src/test/java/org/apache/drill/test/ClusterTest.java:
##
@@ -125,4 +132,56 @@ public static void run(String query, Object... args) 
throws Exception {
   public QueryBuilder queryBuilder( ) {
 return client.queryBuilder();
   }
+
+  /**
+   * Utility method which tests given query produces a {@link UserException} 
and the exception message contains
+   * the given message.
+   * @param testSqlQuery Test query
+   * @param expectedErrorMsg Expected error message.
+   */
+  protected static void errorMsgTestHelper(String testSqlQuery, String 
expectedErrorMsg) throws Exception {
+try {
+  run(testSqlQuery);
+  fail("Expected a UserException when running " + testSqlQuery);
+} catch (UserException actualException) {
+  try {
+assertThat("message of UserException when running " + testSqlQuery, 
actualException.getMessage(), containsString(expectedErrorMsg));
+  } catch (AssertionError e) {
+e.addSuppressed(actualException);
+throw e;
+  }
+}
+  }
+
+  protected static void updateClient(Properties properties) {
+if (client != null) {
+  client.close();
+  client = null;
+}
+ClientFixture.ClientBuilder clientBuilder = cluster.clientBuilder();
+if (properties != null) {
+  for (final String key : properties.stringPropertyNames()) {
+final String lowerCaseKey = key.toLowerCase();
+clientBuilder.property(lowerCaseKey, properties.getProperty(key));
+  }
+}
+client = clientBuilder.build();
+  }
+
+  protected static void updateClient(final String user) {
+updateClient(user, null);
+  }
+
+  protected static void updateClient(final String user, final String password) 
{
+if (client != null) {
+  client.close();
+  client = null;
+}
+final Properties properties = new Properties();
+properties.setProperty(DrillProperties.USER, user);
+if (password != null) {
+  properties.setProperty(DrillProperties.PASSWORD, password);
+}
+updateClient(properties);
+  }

Review Comment:
   Let me test out a couple of things out locally. If they work I'll send a PR 
to you, otherwise we'll be ready to merge as is.



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] vvysotskyi commented on a diff in pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

vvysotskyi commented on code in PR #2599:
URL: https://github.com/apache/drill/pull/2599#discussion_r1096581680


##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillAddAggForExceptRule.java:
##
@@ -0,0 +1,76 @@
+/*
+ * 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 org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelOptRuleOperand;
+import org.apache.calcite.plan.hep.HepRelVertex;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.trace.CalciteTrace;
+import org.apache.curator.shaded.com.google.common.collect.ImmutableList;
+import org.apache.drill.exec.planner.physical.PrelUtil;
+import org.slf4j.Logger;
+
+import static org.apache.drill.exec.ExecConstants.EXCEPT_ADD_AGG_BELOW;
+
+/**
+ * Rule that try to add agg for Except set op.
+ */
+public class DrillAddAggForExceptRule extends RelOptRule {
+  public static final RelOptRule INSTANCE = new 
DrillAddAggForExceptRule(RelOptHelper.any(DrillExceptRel.class), 
"DrillAddAggForExceptRule");
+  protected static final Logger tracer = CalciteTrace.getPlannerTracer();
+
+  public DrillAddAggForExceptRule(RelOptRuleOperand operand, String 
description) {
+super(operand, description);
+  }
+
+  @Override
+  public boolean matches(RelOptRuleCall call) {
+DrillExceptRel drillExceptRel = call.rel(0);
+return !drillExceptRel.all && !drillExceptRel.isAggAdded() && 
!findAggRel(drillExceptRel.getInput(0));

Review Comment:
   I'm not sure whether this check would work properly in some cases. For 
example, the volcano planner will use RelSet to wrap nodes, and perhaps there 
are some other cases. Instead, I propose using 
`RelMetadataQuery.getUniqueKeys()` to ensure that input columns have unique 
values, and if it is so, do not add aggregate. It calls methods from 
`org.apache.calcite.rel.metadata.RelMdUniqueKeys` for specific node types and 
should handle more cases than existing checks. In this case, the `isAggAdded` 
field wouldn't be required.



##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillAddAggForExceptRule.java:
##
@@ -0,0 +1,76 @@
+/*
+ * 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 org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelOptRuleOperand;
+import org.apache.calcite.plan.hep.HepRelVertex;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.trace.CalciteTrace;
+import org.apache.curator.shaded.com.google.common.collect.ImmutableList;
+import org.apache.drill.exec.planner.physical.PrelUtil;
+import org.slf4j.Logger;
+
+import static org.apache.drill.exec.ExecConstants.EXCEPT_ADD_AGG_BELOW;
+
+/**
+ * Rule that try to add agg for Except set op.
+ */
+public class DrillAddAggForExceptRule extends RelOptRule {
+  public static final RelOptRule INSTANCE = new 
DrillAddAggForExceptRule(RelOptHelper.any(DrillExceptRel.class), 
"DrillAddAggForExceptRule");
+  protected static final Logger tracer = CalciteTrace.getPlannerTracer();
+
+  public DrillAddAggForExceptRule(RelOptRuleOperand operand, String 
description) {
+super(operand, description);
+  }
+
+  @Override
+  public bo

[GitHub] [drill] vvysotskyi commented on a diff in pull request #2636: DRILL-8290: Early exit from recursive file listing for LIMIT 0 queries

[via GitHub]

vvysotskyi commented on code in PR #2636:
URL: https://github.com/apache/drill/pull/2636#discussion_r1096586279


##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java:
##
@@ -645,10 +646,18 @@ public Void visitOp(PhysicalOperator op, 
Collection collection
   }
   return null;
 }
-
   }
 
   protected Pair validateNode(SqlNode sqlNode) throws 
ValidationException, RelConversionException, ForemanSetupException {
+if (context.getOptions().getOption(ExecConstants.FILE_LISTING_LIMIT0_OPT)) 
{
+  // Check for a LIMIT 0 in the root portion of the query before validation
+  // because validation of the query's FROM clauses will already trigger
+  // the recursive listing files to which FILE_LISTING_LIMIT0_OPT is meant
+  // to apply.
+  boolean rootSelectLimit0 = FindLimit0SqlVisitor.containsLimit0(sqlNode);
+  context.getPlannerSettings().setRootSelectLimit0(rootSelectLimit0);

Review Comment:
   Perhaps with the query option would be simpler to pass the value rather than 
extend the planner setting interface and pass it through. And schema config 
already has access to query options.



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] kingswanwho commented on a diff in pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

kingswanwho commented on code in PR #2499:
URL: https://github.com/apache/drill/pull/2499#discussion_r1096674511


##
exec/java-exec/src/test/java/org/apache/drill/test/ClusterTest.java:
##
@@ -125,4 +132,56 @@ public static void run(String query, Object... args) 
throws Exception {
   public QueryBuilder queryBuilder( ) {
 return client.queryBuilder();
   }
+
+  /**
+   * Utility method which tests given query produces a {@link UserException} 
and the exception message contains
+   * the given message.
+   * @param testSqlQuery Test query
+   * @param expectedErrorMsg Expected error message.
+   */
+  protected static void errorMsgTestHelper(String testSqlQuery, String 
expectedErrorMsg) throws Exception {
+try {
+  run(testSqlQuery);
+  fail("Expected a UserException when running " + testSqlQuery);
+} catch (UserException actualException) {
+  try {
+assertThat("message of UserException when running " + testSqlQuery, 
actualException.getMessage(), containsString(expectedErrorMsg));
+  } catch (AssertionError e) {
+e.addSuppressed(actualException);
+throw e;
+  }
+}
+  }
+
+  protected static void updateClient(Properties properties) {
+if (client != null) {
+  client.close();
+  client = null;
+}
+ClientFixture.ClientBuilder clientBuilder = cluster.clientBuilder();
+if (properties != null) {
+  for (final String key : properties.stringPropertyNames()) {
+final String lowerCaseKey = key.toLowerCase();
+clientBuilder.property(lowerCaseKey, properties.getProperty(key));
+  }
+}
+client = clientBuilder.build();
+  }
+
+  protected static void updateClient(final String user) {
+updateClient(user, null);
+  }
+
+  protected static void updateClient(final String user, final String password) 
{
+if (client != null) {
+  client.close();
+  client = null;
+}
+final Properties properties = new Properties();
+properties.setProperty(DrillProperties.USER, user);
+if (password != null) {
+  properties.setProperty(DrillProperties.PASSWORD, password);
+}
+updateClient(properties);
+  }

Review Comment:
   Thanks James! I have tried cluster.addClientFixture(), due to this method 
doesn't copy all the client properties from ClusterFixture, so after create a 
new ClientFixture, it still has problem to connect with cluster. And I also 
tried to modify updateClient method signature to return a client, however due 
to the cluster started by startCluster() is hold by a static variable, and 
updateClient should be a non-static method to refactor to ClientFixture, and 
seems a non-static method return a value to static variable made some error 
here, and I am still figuring it out. I think the feasible way should be start 
ClientFixture and ClusterFixture separately in each case, so that those two 
variables are both non-static. The drawback the is the code change should be a 
little too much, but I can submit it commit to see whether it works
   



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] kingswanwho commented on a diff in pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

kingswanwho commented on code in PR #2499:
URL: https://github.com/apache/drill/pull/2499#discussion_r1096674511


##
exec/java-exec/src/test/java/org/apache/drill/test/ClusterTest.java:
##
@@ -125,4 +132,56 @@ public static void run(String query, Object... args) 
throws Exception {
   public QueryBuilder queryBuilder( ) {
 return client.queryBuilder();
   }
+
+  /**
+   * Utility method which tests given query produces a {@link UserException} 
and the exception message contains
+   * the given message.
+   * @param testSqlQuery Test query
+   * @param expectedErrorMsg Expected error message.
+   */
+  protected static void errorMsgTestHelper(String testSqlQuery, String 
expectedErrorMsg) throws Exception {
+try {
+  run(testSqlQuery);
+  fail("Expected a UserException when running " + testSqlQuery);
+} catch (UserException actualException) {
+  try {
+assertThat("message of UserException when running " + testSqlQuery, 
actualException.getMessage(), containsString(expectedErrorMsg));
+  } catch (AssertionError e) {
+e.addSuppressed(actualException);
+throw e;
+  }
+}
+  }
+
+  protected static void updateClient(Properties properties) {
+if (client != null) {
+  client.close();
+  client = null;
+}
+ClientFixture.ClientBuilder clientBuilder = cluster.clientBuilder();
+if (properties != null) {
+  for (final String key : properties.stringPropertyNames()) {
+final String lowerCaseKey = key.toLowerCase();
+clientBuilder.property(lowerCaseKey, properties.getProperty(key));
+  }
+}
+client = clientBuilder.build();
+  }
+
+  protected static void updateClient(final String user) {
+updateClient(user, null);
+  }
+
+  protected static void updateClient(final String user, final String password) 
{
+if (client != null) {
+  client.close();
+  client = null;
+}
+final Properties properties = new Properties();
+properties.setProperty(DrillProperties.USER, user);
+if (password != null) {
+  properties.setProperty(DrillProperties.PASSWORD, password);
+}
+updateClient(properties);
+  }

Review Comment:
   Thanks James! I have tried cluster.addClientFixture(), due to this method 
doesn't copy all the client properties from ClusterFixture, so after create a 
new ClientFixture, it still has problem to connect with ClusterFixture. And I 
also tried to modify updateClient method signature to return a ClientFixture, 
however due to the ClusterFixture and ClientFixture started by startCluster() 
are hold by two static variables, and updateClient should be a non-static 
method to refactor into ClientFixture, seems a non-static method return a value 
to static variable made some errors here, and I am still figuring it out. I 
think the feasible way should be started ClientFixture and ClusterFixture 
separately in each test case, so that those two variables are both non-static. 
The drawback here is the code change should be a little too much, but I can 
submit a commit to see whether it works
   



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre opened a new pull request, #2748: DRILL-8395: Add Support for INSERT and Drop Table to GoogleSheets Plugin

[via GitHub]

cgivre opened a new pull request, #2748:
URL: https://github.com/apache/drill/pull/2748

   # [DRILL-8395](https://issues.apache.org/jira/browse/DRILL-8395): Add 
Support for INSERT and Drop Table to GoogleSheets Plugin
   
   
   ## Description
   This PR adds support for INSERT queries which allow a user to append data to 
an existing GoogleSheets tab.  It also:
   
   * Adds support for DROP TABLE queries which were not implemented
   * Modifies CTAS queries so that if a user executes a CTAS query with a file 
token, Drill will add a new tab to an existing document, but if the user 
executes a CTAS with a file name, it will create an entirely new document.
   
   
   ## Documentation
   Updated README.
   
   ## Testing
   Added unit tests and tested manually. 


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] Leon-WTF commented on a diff in pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

Leon-WTF commented on code in PR #2599:
URL: https://github.com/apache/drill/pull/2599#discussion_r1096965727


##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillAddAggForExceptRule.java:
##
@@ -0,0 +1,76 @@
+/*
+ * 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 org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelOptRuleOperand;
+import org.apache.calcite.plan.hep.HepRelVertex;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.trace.CalciteTrace;
+import org.apache.curator.shaded.com.google.common.collect.ImmutableList;
+import org.apache.drill.exec.planner.physical.PrelUtil;
+import org.slf4j.Logger;
+
+import static org.apache.drill.exec.ExecConstants.EXCEPT_ADD_AGG_BELOW;
+
+/**
+ * Rule that try to add agg for Except set op.
+ */
+public class DrillAddAggForExceptRule extends RelOptRule {
+  public static final RelOptRule INSTANCE = new 
DrillAddAggForExceptRule(RelOptHelper.any(DrillExceptRel.class), 
"DrillAddAggForExceptRule");
+  protected static final Logger tracer = CalciteTrace.getPlannerTracer();
+
+  public DrillAddAggForExceptRule(RelOptRuleOperand operand, String 
description) {
+super(operand, description);
+  }
+
+  @Override
+  public boolean matches(RelOptRuleCall call) {
+DrillExceptRel drillExceptRel = call.rel(0);
+return !drillExceptRel.all && !drillExceptRel.isAggAdded() && 
!findAggRel(drillExceptRel.getInput(0));
+  }
+
+  private boolean findAggRel(RelNode relNode) {
+if (relNode instanceof HepRelVertex) {
+  return findAggRel(((HepRelVertex) relNode).getCurrentRel());
+}
+if (relNode instanceof DrillAggregateRel) {
+  return true;
+}
+if (relNode.getInputs().size() == 1 && relNode.getInput(0) != null) {
+  return findAggRel(relNode.getInput(0));
+}
+return false;
+  }
+
+  @Override
+  public void onMatch(RelOptRuleCall call) {
+final DrillExceptRel drillExceptRel = call.rel(0);
+boolean addAggBelow = 
PrelUtil.getPlannerSettings(call.getPlanner()).getOptions().getOption(EXCEPT_ADD_AGG_BELOW);
+if (addAggBelow) {
+  RelNode aggNode = new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.getInput(0),
+ImmutableBitSet.range(0, 
drillExceptRel.getInput(0).getRowType().getFieldList().size()), 
ImmutableList.of(), ImmutableList.of());
+  call.transformTo(drillExceptRel.copy(ImmutableList.of(aggNode, 
drillExceptRel.getInput(1)), true));
+} else {
+  call.transformTo(new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.copy(true),

Review Comment:
   It's for performance, if the data cardinality is high, aggregate before 
except may not reduce many data, if the data after except left are few, 
aggregate after except will only handle few data which is faster than before 
except.



##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillAddAggForExceptRule.java:
##
@@ -0,0 +1,76 @@
+/*
+ * 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 org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelOptRuleOperand;
+import org.apache.cal

[GitHub] [drill] Leon-WTF commented on a diff in pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

Leon-WTF commented on code in PR #2599:
URL: https://github.com/apache/drill/pull/2599#discussion_r1096965727


##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillAddAggForExceptRule.java:
##
@@ -0,0 +1,76 @@
+/*
+ * 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 org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelOptRuleOperand;
+import org.apache.calcite.plan.hep.HepRelVertex;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.trace.CalciteTrace;
+import org.apache.curator.shaded.com.google.common.collect.ImmutableList;
+import org.apache.drill.exec.planner.physical.PrelUtil;
+import org.slf4j.Logger;
+
+import static org.apache.drill.exec.ExecConstants.EXCEPT_ADD_AGG_BELOW;
+
+/**
+ * Rule that try to add agg for Except set op.
+ */
+public class DrillAddAggForExceptRule extends RelOptRule {
+  public static final RelOptRule INSTANCE = new 
DrillAddAggForExceptRule(RelOptHelper.any(DrillExceptRel.class), 
"DrillAddAggForExceptRule");
+  protected static final Logger tracer = CalciteTrace.getPlannerTracer();
+
+  public DrillAddAggForExceptRule(RelOptRuleOperand operand, String 
description) {
+super(operand, description);
+  }
+
+  @Override
+  public boolean matches(RelOptRuleCall call) {
+DrillExceptRel drillExceptRel = call.rel(0);
+return !drillExceptRel.all && !drillExceptRel.isAggAdded() && 
!findAggRel(drillExceptRel.getInput(0));
+  }
+
+  private boolean findAggRel(RelNode relNode) {
+if (relNode instanceof HepRelVertex) {
+  return findAggRel(((HepRelVertex) relNode).getCurrentRel());
+}
+if (relNode instanceof DrillAggregateRel) {
+  return true;
+}
+if (relNode.getInputs().size() == 1 && relNode.getInput(0) != null) {
+  return findAggRel(relNode.getInput(0));
+}
+return false;
+  }
+
+  @Override
+  public void onMatch(RelOptRuleCall call) {
+final DrillExceptRel drillExceptRel = call.rel(0);
+boolean addAggBelow = 
PrelUtil.getPlannerSettings(call.getPlanner()).getOptions().getOption(EXCEPT_ADD_AGG_BELOW);
+if (addAggBelow) {
+  RelNode aggNode = new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.getInput(0),
+ImmutableBitSet.range(0, 
drillExceptRel.getInput(0).getRowType().getFieldList().size()), 
ImmutableList.of(), ImmutableList.of());
+  call.transformTo(drillExceptRel.copy(ImmutableList.of(aggNode, 
drillExceptRel.getInput(1)), true));
+} else {
+  call.transformTo(new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.copy(true),

Review Comment:
   @vvysotskyi It's for performance, if the data cardinality is high, aggregate 
before except may not reduce many data, if the data after except left are few, 
aggregate after except will only handle few data which is faster than before 
except. This may be choosen by statistics info + CBO automaticlly in the future.



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] vvysotskyi commented on a diff in pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

vvysotskyi commented on code in PR #2599:
URL: https://github.com/apache/drill/pull/2599#discussion_r1097000816


##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillAddAggForExceptRule.java:
##
@@ -0,0 +1,76 @@
+/*
+ * 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 org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelOptRuleOperand;
+import org.apache.calcite.plan.hep.HepRelVertex;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.trace.CalciteTrace;
+import org.apache.curator.shaded.com.google.common.collect.ImmutableList;
+import org.apache.drill.exec.planner.physical.PrelUtil;
+import org.slf4j.Logger;
+
+import static org.apache.drill.exec.ExecConstants.EXCEPT_ADD_AGG_BELOW;
+
+/**
+ * Rule that try to add agg for Except set op.
+ */
+public class DrillAddAggForExceptRule extends RelOptRule {
+  public static final RelOptRule INSTANCE = new 
DrillAddAggForExceptRule(RelOptHelper.any(DrillExceptRel.class), 
"DrillAddAggForExceptRule");
+  protected static final Logger tracer = CalciteTrace.getPlannerTracer();
+
+  public DrillAddAggForExceptRule(RelOptRuleOperand operand, String 
description) {
+super(operand, description);
+  }
+
+  @Override
+  public boolean matches(RelOptRuleCall call) {
+DrillExceptRel drillExceptRel = call.rel(0);
+return !drillExceptRel.all && !drillExceptRel.isAggAdded() && 
!findAggRel(drillExceptRel.getInput(0));
+  }
+
+  private boolean findAggRel(RelNode relNode) {
+if (relNode instanceof HepRelVertex) {
+  return findAggRel(((HepRelVertex) relNode).getCurrentRel());
+}
+if (relNode instanceof DrillAggregateRel) {
+  return true;
+}
+if (relNode.getInputs().size() == 1 && relNode.getInput(0) != null) {
+  return findAggRel(relNode.getInput(0));
+}
+return false;
+  }
+
+  @Override
+  public void onMatch(RelOptRuleCall call) {
+final DrillExceptRel drillExceptRel = call.rel(0);
+boolean addAggBelow = 
PrelUtil.getPlannerSettings(call.getPlanner()).getOptions().getOption(EXCEPT_ADD_AGG_BELOW);
+if (addAggBelow) {
+  RelNode aggNode = new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.getInput(0),
+ImmutableBitSet.range(0, 
drillExceptRel.getInput(0).getRowType().getFieldList().size()), 
ImmutableList.of(), ImmutableList.of());
+  call.transformTo(drillExceptRel.copy(ImmutableList.of(aggNode, 
drillExceptRel.getInput(1)), true));
+} else {
+  call.transformTo(new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.copy(true),

Review Comment:
   But output values should be already distinct after the execution of except 
operator, so the aggregate will do nothing.



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] Leon-WTF commented on a diff in pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

Leon-WTF commented on code in PR #2599:
URL: https://github.com/apache/drill/pull/2599#discussion_r1097007805


##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillAddAggForExceptRule.java:
##
@@ -0,0 +1,76 @@
+/*
+ * 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 org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelOptRuleOperand;
+import org.apache.calcite.plan.hep.HepRelVertex;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.trace.CalciteTrace;
+import org.apache.curator.shaded.com.google.common.collect.ImmutableList;
+import org.apache.drill.exec.planner.physical.PrelUtil;
+import org.slf4j.Logger;
+
+import static org.apache.drill.exec.ExecConstants.EXCEPT_ADD_AGG_BELOW;
+
+/**
+ * Rule that try to add agg for Except set op.
+ */
+public class DrillAddAggForExceptRule extends RelOptRule {
+  public static final RelOptRule INSTANCE = new 
DrillAddAggForExceptRule(RelOptHelper.any(DrillExceptRel.class), 
"DrillAddAggForExceptRule");
+  protected static final Logger tracer = CalciteTrace.getPlannerTracer();
+
+  public DrillAddAggForExceptRule(RelOptRuleOperand operand, String 
description) {
+super(operand, description);
+  }
+
+  @Override
+  public boolean matches(RelOptRuleCall call) {
+DrillExceptRel drillExceptRel = call.rel(0);
+return !drillExceptRel.all && !drillExceptRel.isAggAdded() && 
!findAggRel(drillExceptRel.getInput(0));
+  }
+
+  private boolean findAggRel(RelNode relNode) {
+if (relNode instanceof HepRelVertex) {
+  return findAggRel(((HepRelVertex) relNode).getCurrentRel());
+}
+if (relNode instanceof DrillAggregateRel) {
+  return true;
+}
+if (relNode.getInputs().size() == 1 && relNode.getInput(0) != null) {
+  return findAggRel(relNode.getInput(0));
+}
+return false;
+  }
+
+  @Override
+  public void onMatch(RelOptRuleCall call) {
+final DrillExceptRel drillExceptRel = call.rel(0);
+boolean addAggBelow = 
PrelUtil.getPlannerSettings(call.getPlanner()).getOptions().getOption(EXCEPT_ADD_AGG_BELOW);
+if (addAggBelow) {
+  RelNode aggNode = new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.getInput(0),
+ImmutableBitSet.range(0, 
drillExceptRel.getInput(0).getRowType().getFieldList().size()), 
ImmutableList.of(), ImmutableList.of());
+  call.transformTo(drillExceptRel.copy(ImmutableList.of(aggNode, 
drillExceptRel.getInput(1)), true));
+} else {
+  call.transformTo(new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.copy(true),

Review Comment:
   It's not distinct for left table after except operator.



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] Leon-WTF commented on a diff in pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

Leon-WTF commented on code in PR #2599:
URL: https://github.com/apache/drill/pull/2599#discussion_r1097007805


##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillAddAggForExceptRule.java:
##
@@ -0,0 +1,76 @@
+/*
+ * 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 org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelOptRuleOperand;
+import org.apache.calcite.plan.hep.HepRelVertex;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.trace.CalciteTrace;
+import org.apache.curator.shaded.com.google.common.collect.ImmutableList;
+import org.apache.drill.exec.planner.physical.PrelUtil;
+import org.slf4j.Logger;
+
+import static org.apache.drill.exec.ExecConstants.EXCEPT_ADD_AGG_BELOW;
+
+/**
+ * Rule that try to add agg for Except set op.
+ */
+public class DrillAddAggForExceptRule extends RelOptRule {
+  public static final RelOptRule INSTANCE = new 
DrillAddAggForExceptRule(RelOptHelper.any(DrillExceptRel.class), 
"DrillAddAggForExceptRule");
+  protected static final Logger tracer = CalciteTrace.getPlannerTracer();
+
+  public DrillAddAggForExceptRule(RelOptRuleOperand operand, String 
description) {
+super(operand, description);
+  }
+
+  @Override
+  public boolean matches(RelOptRuleCall call) {
+DrillExceptRel drillExceptRel = call.rel(0);
+return !drillExceptRel.all && !drillExceptRel.isAggAdded() && 
!findAggRel(drillExceptRel.getInput(0));
+  }
+
+  private boolean findAggRel(RelNode relNode) {
+if (relNode instanceof HepRelVertex) {
+  return findAggRel(((HepRelVertex) relNode).getCurrentRel());
+}
+if (relNode instanceof DrillAggregateRel) {
+  return true;
+}
+if (relNode.getInputs().size() == 1 && relNode.getInput(0) != null) {
+  return findAggRel(relNode.getInput(0));
+}
+return false;
+  }
+
+  @Override
+  public void onMatch(RelOptRuleCall call) {
+final DrillExceptRel drillExceptRel = call.rel(0);
+boolean addAggBelow = 
PrelUtil.getPlannerSettings(call.getPlanner()).getOptions().getOption(EXCEPT_ADD_AGG_BELOW);
+if (addAggBelow) {
+  RelNode aggNode = new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.getInput(0),
+ImmutableBitSet.range(0, 
drillExceptRel.getInput(0).getRowType().getFieldList().size()), 
ImmutableList.of(), ImmutableList.of());
+  call.transformTo(drillExceptRel.copy(ImmutableList.of(aggNode, 
drillExceptRel.getInput(1)), true));
+} else {
+  call.transformTo(new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.copy(true),

Review Comment:
   It's not distinct for left table after except operator. I choosed to reuse 
an aggregate operator to do the distinct.



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] vvysotskyi commented on a diff in pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

vvysotskyi commented on code in PR #2599:
URL: https://github.com/apache/drill/pull/2599#discussion_r1097057876


##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillAddAggForExceptRule.java:
##
@@ -0,0 +1,76 @@
+/*
+ * 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 org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelOptRuleOperand;
+import org.apache.calcite.plan.hep.HepRelVertex;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.trace.CalciteTrace;
+import org.apache.curator.shaded.com.google.common.collect.ImmutableList;
+import org.apache.drill.exec.planner.physical.PrelUtil;
+import org.slf4j.Logger;
+
+import static org.apache.drill.exec.ExecConstants.EXCEPT_ADD_AGG_BELOW;
+
+/**
+ * Rule that try to add agg for Except set op.
+ */
+public class DrillAddAggForExceptRule extends RelOptRule {
+  public static final RelOptRule INSTANCE = new 
DrillAddAggForExceptRule(RelOptHelper.any(DrillExceptRel.class), 
"DrillAddAggForExceptRule");
+  protected static final Logger tracer = CalciteTrace.getPlannerTracer();
+
+  public DrillAddAggForExceptRule(RelOptRuleOperand operand, String 
description) {
+super(operand, description);
+  }
+
+  @Override
+  public boolean matches(RelOptRuleCall call) {
+DrillExceptRel drillExceptRel = call.rel(0);
+return !drillExceptRel.all && !drillExceptRel.isAggAdded() && 
!findAggRel(drillExceptRel.getInput(0));
+  }
+
+  private boolean findAggRel(RelNode relNode) {
+if (relNode instanceof HepRelVertex) {
+  return findAggRel(((HepRelVertex) relNode).getCurrentRel());
+}
+if (relNode instanceof DrillAggregateRel) {
+  return true;
+}
+if (relNode.getInputs().size() == 1 && relNode.getInput(0) != null) {
+  return findAggRel(relNode.getInput(0));
+}
+return false;
+  }
+
+  @Override
+  public void onMatch(RelOptRuleCall call) {
+final DrillExceptRel drillExceptRel = call.rel(0);
+boolean addAggBelow = 
PrelUtil.getPlannerSettings(call.getPlanner()).getOptions().getOption(EXCEPT_ADD_AGG_BELOW);
+if (addAggBelow) {
+  RelNode aggNode = new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.getInput(0),
+ImmutableBitSet.range(0, 
drillExceptRel.getInput(0).getRowType().getFieldList().size()), 
ImmutableList.of(), ImmutableList.of());
+  call.transformTo(drillExceptRel.copy(ImmutableList.of(aggNode, 
drillExceptRel.getInput(1)), true));
+} else {
+  call.transformTo(new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.copy(true),

Review Comment:
   Oh, ok, if it is specific to our implementation of except operator, 
aggregation added here possibly could be removed by other Calcite rules which 
assume that results would be already distinct.
   
   I think it would be better to add an aggregation when converting it to 
physical rel nodes.



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] Leon-WTF commented on a diff in pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

Leon-WTF commented on code in PR #2599:
URL: https://github.com/apache/drill/pull/2599#discussion_r1097148776


##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillAddAggForExceptRule.java:
##
@@ -0,0 +1,76 @@
+/*
+ * 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 org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelOptRuleOperand;
+import org.apache.calcite.plan.hep.HepRelVertex;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.trace.CalciteTrace;
+import org.apache.curator.shaded.com.google.common.collect.ImmutableList;
+import org.apache.drill.exec.planner.physical.PrelUtil;
+import org.slf4j.Logger;
+
+import static org.apache.drill.exec.ExecConstants.EXCEPT_ADD_AGG_BELOW;
+
+/**
+ * Rule that try to add agg for Except set op.
+ */
+public class DrillAddAggForExceptRule extends RelOptRule {
+  public static final RelOptRule INSTANCE = new 
DrillAddAggForExceptRule(RelOptHelper.any(DrillExceptRel.class), 
"DrillAddAggForExceptRule");
+  protected static final Logger tracer = CalciteTrace.getPlannerTracer();
+
+  public DrillAddAggForExceptRule(RelOptRuleOperand operand, String 
description) {
+super(operand, description);
+  }
+
+  @Override
+  public boolean matches(RelOptRuleCall call) {
+DrillExceptRel drillExceptRel = call.rel(0);
+return !drillExceptRel.all && !drillExceptRel.isAggAdded() && 
!findAggRel(drillExceptRel.getInput(0));
+  }
+
+  private boolean findAggRel(RelNode relNode) {
+if (relNode instanceof HepRelVertex) {
+  return findAggRel(((HepRelVertex) relNode).getCurrentRel());
+}
+if (relNode instanceof DrillAggregateRel) {
+  return true;
+}
+if (relNode.getInputs().size() == 1 && relNode.getInput(0) != null) {
+  return findAggRel(relNode.getInput(0));
+}
+return false;
+  }
+
+  @Override
+  public void onMatch(RelOptRuleCall call) {
+final DrillExceptRel drillExceptRel = call.rel(0);
+boolean addAggBelow = 
PrelUtil.getPlannerSettings(call.getPlanner()).getOptions().getOption(EXCEPT_ADD_AGG_BELOW);
+if (addAggBelow) {
+  RelNode aggNode = new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.getInput(0),
+ImmutableBitSet.range(0, 
drillExceptRel.getInput(0).getRowType().getFieldList().size()), 
ImmutableList.of(), ImmutableList.of());
+  call.transformTo(drillExceptRel.copy(ImmutableList.of(aggNode, 
drillExceptRel.getInput(1)), true));
+} else {
+  call.transformTo(new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.copy(true),

Review Comment:
   oh, yes, I will refactor 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.

To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] kingswanwho commented on a diff in pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

kingswanwho commented on code in PR #2499:
URL: https://github.com/apache/drill/pull/2499#discussion_r1097151371


##
exec/java-exec/src/test/java/org/apache/drill/test/ClusterTest.java:
##
@@ -125,4 +132,56 @@ public static void run(String query, Object... args) 
throws Exception {
   public QueryBuilder queryBuilder( ) {
 return client.queryBuilder();
   }
+
+  /**
+   * Utility method which tests given query produces a {@link UserException} 
and the exception message contains
+   * the given message.
+   * @param testSqlQuery Test query
+   * @param expectedErrorMsg Expected error message.
+   */
+  protected static void errorMsgTestHelper(String testSqlQuery, String 
expectedErrorMsg) throws Exception {
+try {
+  run(testSqlQuery);
+  fail("Expected a UserException when running " + testSqlQuery);
+} catch (UserException actualException) {
+  try {
+assertThat("message of UserException when running " + testSqlQuery, 
actualException.getMessage(), containsString(expectedErrorMsg));
+  } catch (AssertionError e) {
+e.addSuppressed(actualException);
+throw e;
+  }
+}
+  }
+
+  protected static void updateClient(Properties properties) {
+if (client != null) {
+  client.close();
+  client = null;
+}
+ClientFixture.ClientBuilder clientBuilder = cluster.clientBuilder();
+if (properties != null) {
+  for (final String key : properties.stringPropertyNames()) {
+final String lowerCaseKey = key.toLowerCase();
+clientBuilder.property(lowerCaseKey, properties.getProperty(key));
+  }
+}
+client = clientBuilder.build();
+  }
+
+  protected static void updateClient(final String user) {
+updateClient(user, null);
+  }
+
+  protected static void updateClient(final String user, final String password) 
{
+if (client != null) {
+  client.close();
+  client = null;
+}
+final Properties properties = new Properties();
+properties.setProperty(DrillProperties.USER, user);
+if (password != null) {
+  properties.setProperty(DrillProperties.PASSWORD, password);
+}
+updateClient(properties);
+  }

Review Comment:
   Hi James, I pushed a new commit which refactor updateClient from ClusterTest 
to ClientFixture, and I can pass the client into updateClient so that we don't 
need to client.close for each updateClient, how method signature needs add  
another variable specifically for close client. 



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] kingswanwho commented on a diff in pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

kingswanwho commented on code in PR #2499:
URL: https://github.com/apache/drill/pull/2499#discussion_r1097151371


##
exec/java-exec/src/test/java/org/apache/drill/test/ClusterTest.java:
##
@@ -125,4 +132,56 @@ public static void run(String query, Object... args) 
throws Exception {
   public QueryBuilder queryBuilder( ) {
 return client.queryBuilder();
   }
+
+  /**
+   * Utility method which tests given query produces a {@link UserException} 
and the exception message contains
+   * the given message.
+   * @param testSqlQuery Test query
+   * @param expectedErrorMsg Expected error message.
+   */
+  protected static void errorMsgTestHelper(String testSqlQuery, String 
expectedErrorMsg) throws Exception {
+try {
+  run(testSqlQuery);
+  fail("Expected a UserException when running " + testSqlQuery);
+} catch (UserException actualException) {
+  try {
+assertThat("message of UserException when running " + testSqlQuery, 
actualException.getMessage(), containsString(expectedErrorMsg));
+  } catch (AssertionError e) {
+e.addSuppressed(actualException);
+throw e;
+  }
+}
+  }
+
+  protected static void updateClient(Properties properties) {
+if (client != null) {
+  client.close();
+  client = null;
+}
+ClientFixture.ClientBuilder clientBuilder = cluster.clientBuilder();
+if (properties != null) {
+  for (final String key : properties.stringPropertyNames()) {
+final String lowerCaseKey = key.toLowerCase();
+clientBuilder.property(lowerCaseKey, properties.getProperty(key));
+  }
+}
+client = clientBuilder.build();
+  }
+
+  protected static void updateClient(final String user) {
+updateClient(user, null);
+  }
+
+  protected static void updateClient(final String user, final String password) 
{
+if (client != null) {
+  client.close();
+  client = null;
+}
+final Properties properties = new Properties();
+properties.setProperty(DrillProperties.USER, user);
+if (password != null) {
+  properties.setProperty(DrillProperties.PASSWORD, password);
+}
+updateClient(properties);
+  }

Review Comment:
   Hi James, I pushed a new commit which refactor updateClient from ClusterTest 
to ClientFixture, and I can pass the client into updateClient so that we don't 
need to client.close for each updateClient call, however method signature needs 
add another variable specifically for close client. 



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] kingswanwho commented on a diff in pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

kingswanwho commented on code in PR #2499:
URL: https://github.com/apache/drill/pull/2499#discussion_r1097151371


##
exec/java-exec/src/test/java/org/apache/drill/test/ClusterTest.java:
##
@@ -125,4 +132,56 @@ public static void run(String query, Object... args) 
throws Exception {
   public QueryBuilder queryBuilder( ) {
 return client.queryBuilder();
   }
+
+  /**
+   * Utility method which tests given query produces a {@link UserException} 
and the exception message contains
+   * the given message.
+   * @param testSqlQuery Test query
+   * @param expectedErrorMsg Expected error message.
+   */
+  protected static void errorMsgTestHelper(String testSqlQuery, String 
expectedErrorMsg) throws Exception {
+try {
+  run(testSqlQuery);
+  fail("Expected a UserException when running " + testSqlQuery);
+} catch (UserException actualException) {
+  try {
+assertThat("message of UserException when running " + testSqlQuery, 
actualException.getMessage(), containsString(expectedErrorMsg));
+  } catch (AssertionError e) {
+e.addSuppressed(actualException);
+throw e;
+  }
+}
+  }
+
+  protected static void updateClient(Properties properties) {
+if (client != null) {
+  client.close();
+  client = null;
+}
+ClientFixture.ClientBuilder clientBuilder = cluster.clientBuilder();
+if (properties != null) {
+  for (final String key : properties.stringPropertyNames()) {
+final String lowerCaseKey = key.toLowerCase();
+clientBuilder.property(lowerCaseKey, properties.getProperty(key));
+  }
+}
+client = clientBuilder.build();
+  }
+
+  protected static void updateClient(final String user) {
+updateClient(user, null);
+  }
+
+  protected static void updateClient(final String user, final String password) 
{
+if (client != null) {
+  client.close();
+  client = null;
+}
+final Properties properties = new Properties();
+properties.setProperty(DrillProperties.USER, user);
+if (password != null) {
+  properties.setProperty(DrillProperties.PASSWORD, password);
+}
+updateClient(properties);
+  }

Review Comment:
   Hi James, I pushed a new commit which refactor updateClient from ClusterTest 
to ClientFixture.



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] vvysotskyi opened a new pull request, #2749: DRILL-8379: Update Calcite to 1.33.0

[via GitHub]

vvysotskyi opened a new pull request, #2749:
URL: https://github.com/apache/drill/pull/2749

   # [DRILL-8379](https://issues.apache.org/jira/browse/DRILL-8379): Update 
Calcite to 1.33.0
   
   ## Description
   Updated Caclite and Avatica versions. Had to update Jackson to avoid errors 
with ES Calcite connector.
   
   ## Documentation
   NA
   
   ## Testing
   All unit tests pass.
   


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton commented on a diff in pull request #2749: DRILL-8379: Update Calcite to 1.33.0

[via GitHub]

jnturton commented on code in PR #2749:
URL: https://github.com/apache/drill/pull/2749#discussion_r1098367575


##
exec/java-exec/src/main/codegen/data/Parser.tdd:
##
@@ -118,780 +118,824 @@
   # List of keywords from "keywords" section that are not reserved.
   # Copied from calcite-core config.fmpp
   # For details please see comment under CALCITE-2405.
-   nonReservedKeywords: [
-"A"
-"ABSENT"
-"ABSOLUTE"
-"ACTION"
-"ADA"
-"ADD"
-"ADMIN"
-"AFTER"
-"ALIAS"
-"ALIASES"
-"ALWAYS"
-"APPLY"
-"ASC"
-"ASSERTION"
-"ASSIGNMENT"
-"ATTRIBUTE"
-"ATTRIBUTES"
-"BEFORE"
-"BERNOULLI"
-"BREADTH"
-"C"
-"CASCADE"
-"CATALOG"
-"CATALOG_NAME"
-"CENTURY"
-"CHAIN"
-"CHARACTER_SET_CATALOG"
-"CHARACTER_SET_NAME"
-"CHARACTER_SET_SCHEMA"
-"CHARACTERISTICS"
-"CHARACTERS"
-"CLASS_ORIGIN"
-"COBOL"
-"COLLATION"
-"COLLATION_CATALOG"
-"COLLATION_NAME"
-"COLLATION_SCHEMA"
-"COLUMN_NAME"
-"COMMAND_FUNCTION"
-"COMMAND_FUNCTION_CODE"
-"COMMITTED"
-"CONDITION_NUMBER"
-"CONNECTION"
-"CONNECTION_NAME"
-"CONSTRAINT_CATALOG"
-"CONSTRAINT_NAME"
-"CONSTRAINT_SCHEMA"
-"CONSTRAINTS"
-"CONSTRUCTOR"
-"CONTINUE"
-"CURSOR_NAME"
-"DATA"
-"DATABASE"
-"DATETIME_INTERVAL_CODE"
-"DATETIME_INTERVAL_PRECISION"
-"DECADE"
-"DEFAULTS"
-"DEFERRABLE"
-"DEFERRED"
-"DEFINED"
-"DEFINER"
-"DEGREE"
-"DEPTH"
-"DERIVED"
-"DESC"
-"DESCRIPTION"
-"DESCRIPTOR"
-"DIAGNOSTICS"
-"DISPATCH"
-"DOMAIN"
-"DOW"
-"DOY"
-"DYNAMIC_FUNCTION"
-"DYNAMIC_FUNCTION_CODE"
-"ENCODING"
-"EPOCH"
-"ERROR"
-"EXCEPTION"
-"EXCLUDE"
-"EXCLUDING"
-"FINAL"
-"FIRST"
-"FOLLOWING"
-"FORMAT"
-"FORTRAN"
-"FOUND"
-"FRAC_SECOND"
-"G"
-"GENERAL"
-"GENERATED"
-"GEOMETRY"
-"GO"
-"GOTO"
-"GRANTED"
-"HIERARCHY"
-"IMMEDIATE"
-"IMMEDIATELY"
-"IMPLEMENTATION"
-"INCLUDING"
-"INCREMENT"
-"INITIALLY"
-"INPUT"
-"INSTANCE"
-"INSTANTIABLE"
-"INVOKER"
-"ISODOW"
-"ISOYEAR"
-"ISOLATION"
-"JAVA"
-"JSON"
-"K"
-"KEY"
-"KEY_MEMBER"
-"KEY_TYPE"
-"LABEL"
-"LAST"
-"LENGTH"
-"LEVEL"
-"LIBRARY"
-"LOCATOR"
-"M"
-"MAP"
-"MATCHED"
-"MAXVALUE"
-"MICROSECOND"
-"MESSAGE_LENGTH"
-"MESSAGE_OCTET_LENGTH"
-"MESSAGE_TEXT"
-"MILLISECOND"
-"MILLENNIUM"
-"MINVALUE"
-"MONTHS"
-"MORE_"
-"MUMPS"
-"NAME"
-"NAMES"
-"NANOSECOND"
-"NESTING"
-"NORMALIZED"
-"NULLABLE"
-"NULLS"
-"NUMBER"
-"OBJECT"
-"OCTETS"
-"OPTION"
-"OPTIONS"
-"ORDERING"
-"ORDINALITY"
-"OTHERS"
-"OUTPUT"
-"OVERRIDING"
-"PAD"
-"PARAMETER_MODE"
-"PARAMETER_NAME"
-"PARAMETER_ORDINAL_POSITION"
-"PARAMETER_SPECIFIC_CATALOG"
-"PARAMETER_SPECIFIC_NAME"
-"PARAMETER_SPECIFIC_SCHEMA"
-"PARTIAL"
-"PASCAL"
-"PASSING"
-"PASSTHROUGH"
-"PAST"
-"PATH"
-"PLACING"
-"PLAN"
-"PLI"
-"PRECEDING"
-"PRESERVE"
-"PRIOR"
-"PRIVILEGES"
-"PUBLIC"
-"QUARTER"
-"READ"
-"RELATIVE"
-"REPEATABLE"
-"REPLACE"
-"RESTART"
-"RESTRICT"
-"RETURNED_CARDINALITY"
-"RETURNED_LENGTH"
-"RETURNED_OCTET_LENGTH"
-"RETURNED_SQLSTATE"
-"ROLE"
-"ROUTINE"
-"ROUTINE_CATALOG"
-"ROUTINE_NAME"
-"ROUTINE_SCHEMA"
-"ROW_COUNT"
-"SCALE"
-"SCHEMA"
-"SCHEMA_NAME"
-"SCOPE_CATALOGS"
-"SCOPE_NAME"
-"SCOPE_SCHEMA"
-"SECTION"
-"SECURITY"
-"SELF"
-"SEQUENCE"
-"SERIALIZABLE"
-"SERVER"
-"SERVER_NAME"
-"SESSION"
-"SETS"
-"SIMPLE"
-"SIZE"
-"SOURCE"
-"SPACE"
-"SPECIFIC_NAME"
-"SQL_BIGINT"
-"SQL_BINARY"
-"SQL_BIT"
-"SQL_BLOB"
-"SQL_BOOLEAN"
-"SQL_CHAR"
-"SQL_CLOB"
-"SQL_DATE"
-"SQL_DECIMAL"
-"SQL_DOUBLE"
-"SQL_FLOAT"
- 

[GitHub] [drill] cgivre merged pull request #2739: DRILL-8387: Add Support for User Translation to ElasticSearch Plugin

[via GitHub]

cgivre merged PR #2739:
URL: https://github.com/apache/drill/pull/2739


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] vvysotskyi commented on a diff in pull request #2749: DRILL-8379: Update Calcite to 1.33.0

[via GitHub]

vvysotskyi commented on code in PR #2749:
URL: https://github.com/apache/drill/pull/2749#discussion_r1099017177


##
exec/java-exec/src/main/codegen/data/Parser.tdd:
##
@@ -118,780 +118,824 @@
   # List of keywords from "keywords" section that are not reserved.
   # Copied from calcite-core config.fmpp
   # For details please see comment under CALCITE-2405.
-   nonReservedKeywords: [
-"A"
-"ABSENT"
-"ABSOLUTE"
-"ACTION"
-"ADA"
-"ADD"
-"ADMIN"
-"AFTER"
-"ALIAS"
-"ALIASES"
-"ALWAYS"
-"APPLY"
-"ASC"
-"ASSERTION"
-"ASSIGNMENT"
-"ATTRIBUTE"
-"ATTRIBUTES"
-"BEFORE"
-"BERNOULLI"
-"BREADTH"
-"C"
-"CASCADE"
-"CATALOG"
-"CATALOG_NAME"
-"CENTURY"
-"CHAIN"
-"CHARACTER_SET_CATALOG"
-"CHARACTER_SET_NAME"
-"CHARACTER_SET_SCHEMA"
-"CHARACTERISTICS"
-"CHARACTERS"
-"CLASS_ORIGIN"
-"COBOL"
-"COLLATION"
-"COLLATION_CATALOG"
-"COLLATION_NAME"
-"COLLATION_SCHEMA"
-"COLUMN_NAME"
-"COMMAND_FUNCTION"
-"COMMAND_FUNCTION_CODE"
-"COMMITTED"
-"CONDITION_NUMBER"
-"CONNECTION"
-"CONNECTION_NAME"
-"CONSTRAINT_CATALOG"
-"CONSTRAINT_NAME"
-"CONSTRAINT_SCHEMA"
-"CONSTRAINTS"
-"CONSTRUCTOR"
-"CONTINUE"
-"CURSOR_NAME"
-"DATA"
-"DATABASE"
-"DATETIME_INTERVAL_CODE"
-"DATETIME_INTERVAL_PRECISION"
-"DECADE"
-"DEFAULTS"
-"DEFERRABLE"
-"DEFERRED"
-"DEFINED"
-"DEFINER"
-"DEGREE"
-"DEPTH"
-"DERIVED"
-"DESC"
-"DESCRIPTION"
-"DESCRIPTOR"
-"DIAGNOSTICS"
-"DISPATCH"
-"DOMAIN"
-"DOW"
-"DOY"
-"DYNAMIC_FUNCTION"
-"DYNAMIC_FUNCTION_CODE"
-"ENCODING"
-"EPOCH"
-"ERROR"
-"EXCEPTION"
-"EXCLUDE"
-"EXCLUDING"
-"FINAL"
-"FIRST"
-"FOLLOWING"
-"FORMAT"
-"FORTRAN"
-"FOUND"
-"FRAC_SECOND"
-"G"
-"GENERAL"
-"GENERATED"
-"GEOMETRY"
-"GO"
-"GOTO"
-"GRANTED"
-"HIERARCHY"
-"IMMEDIATE"
-"IMMEDIATELY"
-"IMPLEMENTATION"
-"INCLUDING"
-"INCREMENT"
-"INITIALLY"
-"INPUT"
-"INSTANCE"
-"INSTANTIABLE"
-"INVOKER"
-"ISODOW"
-"ISOYEAR"
-"ISOLATION"
-"JAVA"
-"JSON"
-"K"
-"KEY"
-"KEY_MEMBER"
-"KEY_TYPE"
-"LABEL"
-"LAST"
-"LENGTH"
-"LEVEL"
-"LIBRARY"
-"LOCATOR"
-"M"
-"MAP"
-"MATCHED"
-"MAXVALUE"
-"MICROSECOND"
-"MESSAGE_LENGTH"
-"MESSAGE_OCTET_LENGTH"
-"MESSAGE_TEXT"
-"MILLISECOND"
-"MILLENNIUM"
-"MINVALUE"
-"MONTHS"
-"MORE_"
-"MUMPS"
-"NAME"
-"NAMES"
-"NANOSECOND"
-"NESTING"
-"NORMALIZED"
-"NULLABLE"
-"NULLS"
-"NUMBER"
-"OBJECT"
-"OCTETS"
-"OPTION"
-"OPTIONS"
-"ORDERING"
-"ORDINALITY"
-"OTHERS"
-"OUTPUT"
-"OVERRIDING"
-"PAD"
-"PARAMETER_MODE"
-"PARAMETER_NAME"
-"PARAMETER_ORDINAL_POSITION"
-"PARAMETER_SPECIFIC_CATALOG"
-"PARAMETER_SPECIFIC_NAME"
-"PARAMETER_SPECIFIC_SCHEMA"
-"PARTIAL"
-"PASCAL"
-"PASSING"
-"PASSTHROUGH"
-"PAST"
-"PATH"
-"PLACING"
-"PLAN"
-"PLI"
-"PRECEDING"
-"PRESERVE"
-"PRIOR"
-"PRIVILEGES"
-"PUBLIC"
-"QUARTER"
-"READ"
-"RELATIVE"
-"REPEATABLE"
-"REPLACE"
-"RESTART"
-"RESTRICT"
-"RETURNED_CARDINALITY"
-"RETURNED_LENGTH"
-"RETURNED_OCTET_LENGTH"
-"RETURNED_SQLSTATE"
-"ROLE"
-"ROUTINE"
-"ROUTINE_CATALOG"
-"ROUTINE_NAME"
-"ROUTINE_SCHEMA"
-"ROW_COUNT"
-"SCALE"
-"SCHEMA"
-"SCHEMA_NAME"
-"SCOPE_CATALOGS"
-"SCOPE_NAME"
-"SCOPE_SCHEMA"
-"SECTION"
-"SECURITY"
-"SELF"
-"SEQUENCE"
-"SERIALIZABLE"
-"SERVER"
-"SERVER_NAME"
-"SESSION"
-"SETS"
-"SIMPLE"
-"SIZE"
-"SOURCE"
-"SPACE"
-"SPECIFIC_NAME"
-"SQL_BIGINT"
-"SQL_BINARY"
-"SQL_BIT"
-"SQL_BLOB"
-"SQL_BOOLEAN"
-"SQL_CHAR"
-"SQL_CLOB"
-"SQL_DATE"
-"SQL_DECIMAL"
-"SQL_DOUBLE"
-"SQL_FLOAT"
-   

[GitHub] [drill] cgivre commented on pull request #2749: DRILL-8379: Update Calcite to 1.33.0

[via GitHub]

cgivre commented on PR #2749:
URL: https://github.com/apache/drill/pull/2749#issuecomment-1421470624

   Thanks @vvysotskyi   This looked like it wasn't quite as painless as going 
from 1.31 to 1.32, but didn't look like it required any major surgery. 


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] vvysotskyi commented on pull request #2749: DRILL-8379: Update Calcite to 1.33.0

[via GitHub]

vvysotskyi commented on PR #2749:
URL: https://github.com/apache/drill/pull/2749#issuecomment-1421566035

   > Thanks @vvysotskyi This looked like it wasn't quite as painless as going 
from 1.31 to 1.32, but didn't look like it required any major surgery.
   
   Agree, this update wasn't trivial and forced us to sacrifice some temporary 
table functionality in https://github.com/apache/drill/pull/2733, but still, it 
was much simpler than jumping from 1.21.0 to 1.31.0. 


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton commented on a diff in pull request #2636: DRILL-8290: Early exit from recursive file listing for LIMIT 0 queries

[via GitHub]

jnturton commented on code in PR #2636:
URL: https://github.com/apache/drill/pull/2636#discussion_r1099737668


##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java:
##
@@ -645,10 +646,18 @@ public Void visitOp(PhysicalOperator op, 
Collection collection
   }
   return null;
 }
-
   }
 
   protected Pair validateNode(SqlNode sqlNode) throws 
ValidationException, RelConversionException, ForemanSetupException {
+if (context.getOptions().getOption(ExecConstants.FILE_LISTING_LIMIT0_OPT)) 
{
+  // Check for a LIMIT 0 in the root portion of the query before validation
+  // because validation of the query's FROM clauses will already trigger
+  // the recursive listing files to which FILE_LISTING_LIMIT0_OPT is meant
+  // to apply.
+  boolean rootSelectLimit0 = FindLimit0SqlVisitor.containsLimit0(sqlNode);
+  context.getPlannerSettings().setRootSelectLimit0(rootSelectLimit0);

Review Comment:
   @vvysotskyi I've made use of a query option, how do things look now?



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton merged pull request #2636: DRILL-8290: Early exit from recursive file listing for LIMIT 0 queries

[via GitHub]

jnturton merged PR #2636:
URL: https://github.com/apache/drill/pull/2636


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton merged pull request #2728: DRILL-8372: Unfreed buffers when running a LIMIT 0 query over delimited text

[via GitHub]

jnturton merged PR #2728:
URL: https://github.com/apache/drill/pull/2728


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton opened a new pull request, #2750: DRILL-8309: Upgrade slf4j to 2.0.x

[via GitHub]

jnturton opened a new pull request, #2750:
URL: https://github.com/apache/drill/pull/2750

   # [DRILL-8309](https://issues.apache.org/jira/browse/DRILL-8309): Upgrade 
slf4j to 2.0.x
   
   ## Description
   
   - logback 1.2.11 -> 1.4.5.
   - log4j 2.18.0 -> 2.19.0.
   - slf4j 1.7.26 -> 2.0.6.
   
   ## Documentation
   N/A
   
   ## Testing
   Run embedded Drill and check its log output.
   


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton commented on a diff in pull request #2749: DRILL-8379: Update Calcite to 1.33.0

[via GitHub]

jnturton commented on code in PR #2749:
URL: https://github.com/apache/drill/pull/2749#discussion_r1099912943


##
exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestAnalyze.java:
##
@@ -332,7 +332,7 @@ public void testUseStatistics() throws Exception {
   query = " select emp.employee_id from dfs.tmp.employeeUseStat emp join 
dfs.tmp.departmentUseStat dept"
   + " on emp.department_id = dept.department_id "
   + " group by emp.employee_id";
-  String[] expectedPlan8 = {"HashAgg\\(group=\\[\\{0\\}\\]\\).*rowcount = 
730.2832515526484,.*",
+  String[] expectedPlan8 = {"HashAgg\\(group=\\[\\{0\\}\\]\\).*rowcount = 
730.2832515526.*",

Review Comment:
   Did we drop the last three digits here to prevent spurious test failures 
resulting from rounding errors?



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] vvysotskyi commented on a diff in pull request #2749: DRILL-8379: Update Calcite to 1.33.0

[via GitHub]

vvysotskyi commented on code in PR #2749:
URL: https://github.com/apache/drill/pull/2749#discussion_r1099920934


##
exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestAnalyze.java:
##
@@ -332,7 +332,7 @@ public void testUseStatistics() throws Exception {
   query = " select emp.employee_id from dfs.tmp.employeeUseStat emp join 
dfs.tmp.departmentUseStat dept"
   + " on emp.department_id = dept.department_id "
   + " group by emp.employee_id";
-  String[] expectedPlan8 = {"HashAgg\\(group=\\[\\{0\\}\\]\\).*rowcount = 
730.2832515526484,.*",
+  String[] expectedPlan8 = {"HashAgg\\(group=\\[\\{0\\}\\]\\).*rowcount = 
730.2832515526.*",

Review Comment:
   Calcite has slightly updated the formula to calculate it. I have removed 
these digits to avoid failures for further potential updates.



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre merged pull request #2749: DRILL-8379: Update Calcite to 1.33.0

[via GitHub]

cgivre merged PR #2749:
URL: https://github.com/apache/drill/pull/2749


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre merged pull request #2748: DRILL-8395: Add Support for Insert and Drop Table to GoogleSheets Plugin

[via GitHub]

cgivre merged PR #2748:
URL: https://github.com/apache/drill/pull/2748


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] vvysotskyi opened a new pull request, #2751: DRILL-8396: Update checkstyle version

[via GitHub]

vvysotskyi opened a new pull request, #2751:
URL: https://github.com/apache/drill/pull/2751

   # [DRILL-8396](https://issues.apache.org/jira/browse/DRILL-8396): Update 
checkstyle version
   
   ## Description
   Update checkstyle version to the latest one
   
   ## Documentation
   NA
   
   ## Testing
   Unit tests pass
   


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre commented on pull request #2083: DRILL-7722: Add reproducer unit test

[via GitHub]

cgivre commented on PR #2083:
URL: https://github.com/apache/drill/pull/2083#issuecomment-1423478202

   The underlying issue was fixed in https://github.com/apache/drill/pull/2602.


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre closed pull request #2083: DRILL-7722: Add reproducer unit test

[via GitHub]

cgivre closed pull request #2083: DRILL-7722: Add reproducer unit test
URL: https://github.com/apache/drill/pull/2083


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre merged pull request #2751: DRILL-8396: Update checkstyle version

[via GitHub]

cgivre merged PR #2751:
URL: https://github.com/apache/drill/pull/2751


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre merged pull request #2750: DRILL-8309: Upgrade slf4j to 2.0.x

[via GitHub]

cgivre merged PR #2750:
URL: https://github.com/apache/drill/pull/2750


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton commented on pull request #2750: DRILL-8309: Upgrade slf4j to 2.0.x

[via GitHub]

jnturton commented on PR #2750:
URL: https://github.com/apache/drill/pull/2750#issuecomment-1423573873

   @pjfanning the upgrade wasn't as smooth as it looks, I fought with it for 
hours. I should have left this PR in draft before retiring yesterday because I 
did want to come back and expand my commentary. The seemingly unrelated 
upgrades and removals came from my chasing after fat jars that bundle slf4j < 
2.0 (and were breaking this upgrade). In the end it was the refactoring of the 
Avatica dependency that really counted.


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] vvysotskyi opened a new pull request, #2752: DRILL-8397: Drill prints warnings to console when starting it

[via GitHub]

vvysotskyi opened a new pull request, #2752:
URL: https://github.com/apache/drill/pull/2752

   # [DRILL-8397](https://issues.apache.org/jira/browse/DRILL-8397): Drill 
prints warnings to console when starting it
   
   ## Description
   Replaced the `level` tag with the attribute, so no extra output is printed 
to the console.
   
   ## Documentation
   NA
   
   ## Testing
   Checked manually
   


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton commented on pull request #2752: DRILL-8397: Drill prints warnings to console when starting it

[via GitHub]

jnturton commented on PR #2752:
URL: https://github.com/apache/drill/pull/2752#issuecomment-1424093562

   Regarding the sporadic failures of the unit test 
testBasicCTASWithScalarDataTypes that have been showing in recent CI runs, and 
now also here, it looks to me like [the Thread.sleep(15_000) that it 
uses](https://github.com/jnturton/drill/blob/83bf04cd815e6c6d22d9ef04550cd8113bd9f905/contrib/storage-splunk/src/test/java/org/apache/drill/exec/store/splunk/SplunkWriterTest.java#L113)
 to wait for an INSERT to go through is not always enough to ensure that Splunk 
will return the record needed for the subsequent schema validation to pass.
   
   Locally I've been able to reproduce both successes and failures of this test 
with a range of sleep durations from 6_000 to 60_000 so I'm not sure if there 
is a magic number here, or what it might be. Stepping back further, I don't 
know if Splunk ever takes a transactional approach to loading data - maybe it 
makes no guarantees beyond "Thanks, I'll index this later"?
   
   CC @cgivre.
   


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton commented on pull request #2752: DRILL-8397: Drill prints warnings to console when starting it

[via GitHub]

jnturton commented on PR #2752:
URL: https://github.com/apache/drill/pull/2752#issuecomment-1424101087

   In any case, that failure is certainly unrelated to this PR which in my 
opinion may be merged.


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre commented on pull request #2752: DRILL-8397: Drill prints warnings to console when starting it

[via GitHub]

cgivre commented on PR #2752:
URL: https://github.com/apache/drill/pull/2752#issuecomment-1424151052

   > 
   
   Given the flakyness of that test, I'm fine with marking that as a "run 
manually" test.  I think you are correct that the indexing doesn't happen 
immediately. 


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre commented on pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

cgivre commented on PR #2599:
URL: https://github.com/apache/drill/pull/2599#issuecomment-1424152751

   Hey @Leon-WTF Any chance you could address @vvysotskyi 's comments soon.   
This is one of the last PRs slated to be merged for the next release.


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton merged pull request #2752: DRILL-8397: Drill prints warnings to console when starting it

[via GitHub]

jnturton merged PR #2752:
URL: https://github.com/apache/drill/pull/2752


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] vvysotskyi opened a new pull request, #2753: DRILL-8396: Checkstyle plugin works only with JDK 11+

[via GitHub]

vvysotskyi opened a new pull request, #2753:
URL: https://github.com/apache/drill/pull/2753

   # [DRILL-8396](https://issues.apache.org/jira/browse/DRILL-8396): Checkstyle 
plugin works only with JDK 11+
   
   ## Description
   As it turned out, Checkstyle 10.x version is supported only on JDK 11+ 
(https://checkstyle.org/#JRE_and_JDK), so it causes failures when attempting to 
build Drill on JDK 8. We didn't notice it because CI doesn't use JDK 8 (though 
it should).
   
   To fix it, I have replaced it with the `checkstyle-backport-jre8` library 
which works fine on JDK 8 and is referenced in the official docs: 
https://checkstyle.org/#Backport
   
   ## Documentation
   NA
   
   ## Testing
   Checked manually.
   


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] vvysotskyi opened a new pull request, #2754: DRILL-8398: Fix GitHub Actions to use proper JDK version

[via GitHub]

vvysotskyi opened a new pull request, #2754:
URL: https://github.com/apache/drill/pull/2754

   # [DRILL-8398](https://issues.apache.org/jira/browse/DRILL-8398): Fix GitHub 
Actions to use proper JDK version
   
   ## Description
   Added `actions/setup-java` action to use dedicated java version for tests 
instead of using provided with the instance one. This PR is based on the commit 
for DRILL-8396 to avoid early failures for JDK 8. 
   
   Need to fix hive tests that were broken earlier (because they run only with 
JDK 8).
   
   ## Documentation
   NA
   
   ## Testing
   TBA
   


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre merged pull request #2753: DRILL-8396: Checkstyle plugin works only with JDK 11+

[via GitHub]

cgivre merged PR #2753:
URL: https://github.com/apache/drill/pull/2753


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] Leon-WTF commented on pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

Leon-WTF commented on PR #2599:
URL: https://github.com/apache/drill/pull/2599#issuecomment-1425212100

   > Hey @Leon-WTF Any chance you could address @vvysotskyi 's comments soon. 
This is one of the last PRs slated to be merged for the next release.
   
   I will do it by this weekend, is that ok?


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] Leon-WTF commented on a diff in pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

Leon-WTF commented on code in PR #2599:
URL: https://github.com/apache/drill/pull/2599#discussion_r1102305963


##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillAddAggForExceptRule.java:
##
@@ -0,0 +1,76 @@
+/*
+ * 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 org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelOptRuleOperand;
+import org.apache.calcite.plan.hep.HepRelVertex;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.trace.CalciteTrace;
+import org.apache.curator.shaded.com.google.common.collect.ImmutableList;
+import org.apache.drill.exec.planner.physical.PrelUtil;
+import org.slf4j.Logger;
+
+import static org.apache.drill.exec.ExecConstants.EXCEPT_ADD_AGG_BELOW;
+
+/**
+ * Rule that try to add agg for Except set op.
+ */
+public class DrillAddAggForExceptRule extends RelOptRule {
+  public static final RelOptRule INSTANCE = new 
DrillAddAggForExceptRule(RelOptHelper.any(DrillExceptRel.class), 
"DrillAddAggForExceptRule");
+  protected static final Logger tracer = CalciteTrace.getPlannerTracer();
+
+  public DrillAddAggForExceptRule(RelOptRuleOperand operand, String 
description) {
+super(operand, description);
+  }
+
+  @Override
+  public boolean matches(RelOptRuleCall call) {
+DrillExceptRel drillExceptRel = call.rel(0);
+return !drillExceptRel.all && !drillExceptRel.isAggAdded() && 
!findAggRel(drillExceptRel.getInput(0));
+  }
+
+  private boolean findAggRel(RelNode relNode) {
+if (relNode instanceof HepRelVertex) {
+  return findAggRel(((HepRelVertex) relNode).getCurrentRel());
+}
+if (relNode instanceof DrillAggregateRel) {
+  return true;
+}
+if (relNode.getInputs().size() == 1 && relNode.getInput(0) != null) {
+  return findAggRel(relNode.getInput(0));
+}
+return false;
+  }
+
+  @Override
+  public void onMatch(RelOptRuleCall call) {
+final DrillExceptRel drillExceptRel = call.rel(0);
+boolean addAggBelow = 
PrelUtil.getPlannerSettings(call.getPlanner()).getOptions().getOption(EXCEPT_ADD_AGG_BELOW);
+if (addAggBelow) {
+  RelNode aggNode = new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.getInput(0),
+ImmutableBitSet.range(0, 
drillExceptRel.getInput(0).getRowType().getFieldList().size()), 
ImmutableList.of(), ImmutableList.of());
+  call.transformTo(drillExceptRel.copy(ImmutableList.of(aggNode, 
drillExceptRel.getInput(1)), true));
+} else {
+  call.transformTo(new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.copy(true),

Review Comment:
   One more question about moving converting rule to physical phase, I need to 
add physical agg rel node, so needs to add both hash(distribute by all 
keys/single key) and stream agg, right?



##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillAddAggForExceptRule.java:
##
@@ -0,0 +1,76 @@
+/*
+ * 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 org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelOptRuleOperand;
+import org.apache.calcite.plan.hep.HepRelVertex;
+import org.apa

[GitHub] [drill] cgivre commented on pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

cgivre commented on PR #2599:
URL: https://github.com/apache/drill/pull/2599#issuecomment-1425760414

   > 
   
   @Leon-WTF This weekend would be great!  Very excited to get this merged.


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre opened a new pull request, #2755: DRILL-8399: MS Access Reader Misinterprets Data Types

[via GitHub]

cgivre opened a new pull request, #2755:
URL: https://github.com/apache/drill/pull/2755

   # [DRILL-8399](https://issues.apache.org/jira/browse/DRILL-8399): MS Access 
Reader Misinterprets Data Types
   
   ## Description
   Minor bug fix.  Drill was misinterpreting some data types in MS Access 
files.  This fixes that.
   
   ## Documentation
   N/A
   
   ## Testing
   Added unit 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.

To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] vvysotskyi commented on a diff in pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

vvysotskyi commented on code in PR #2599:
URL: https://github.com/apache/drill/pull/2599#discussion_r1103702656


##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillAddAggForExceptRule.java:
##
@@ -0,0 +1,76 @@
+/*
+ * 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 org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelOptRuleOperand;
+import org.apache.calcite.plan.hep.HepRelVertex;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.trace.CalciteTrace;
+import org.apache.curator.shaded.com.google.common.collect.ImmutableList;
+import org.apache.drill.exec.planner.physical.PrelUtil;
+import org.slf4j.Logger;
+
+import static org.apache.drill.exec.ExecConstants.EXCEPT_ADD_AGG_BELOW;
+
+/**
+ * Rule that try to add agg for Except set op.
+ */
+public class DrillAddAggForExceptRule extends RelOptRule {
+  public static final RelOptRule INSTANCE = new 
DrillAddAggForExceptRule(RelOptHelper.any(DrillExceptRel.class), 
"DrillAddAggForExceptRule");
+  protected static final Logger tracer = CalciteTrace.getPlannerTracer();
+
+  public DrillAddAggForExceptRule(RelOptRuleOperand operand, String 
description) {
+super(operand, description);
+  }
+
+  @Override
+  public boolean matches(RelOptRuleCall call) {
+DrillExceptRel drillExceptRel = call.rel(0);
+return !drillExceptRel.all && !drillExceptRel.isAggAdded() && 
!findAggRel(drillExceptRel.getInput(0));
+  }
+
+  private boolean findAggRel(RelNode relNode) {
+if (relNode instanceof HepRelVertex) {
+  return findAggRel(((HepRelVertex) relNode).getCurrentRel());
+}
+if (relNode instanceof DrillAggregateRel) {
+  return true;
+}
+if (relNode.getInputs().size() == 1 && relNode.getInput(0) != null) {
+  return findAggRel(relNode.getInput(0));
+}
+return false;
+  }
+
+  @Override
+  public void onMatch(RelOptRuleCall call) {
+final DrillExceptRel drillExceptRel = call.rel(0);
+boolean addAggBelow = 
PrelUtil.getPlannerSettings(call.getPlanner()).getOptions().getOption(EXCEPT_ADD_AGG_BELOW);
+if (addAggBelow) {
+  RelNode aggNode = new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.getInput(0),
+ImmutableBitSet.range(0, 
drillExceptRel.getInput(0).getRowType().getFieldList().size()), 
ImmutableList.of(), ImmutableList.of());
+  call.transformTo(drillExceptRel.copy(ImmutableList.of(aggNode, 
drillExceptRel.getInput(1)), true));
+} else {
+  call.transformTo(new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.copy(true),

Review Comment:
   Drill doesn't use streaming aggregate for distinct calls, so only hash agg 
should be enough.



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] Leon-WTF commented on a diff in pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

Leon-WTF commented on code in PR #2599:
URL: https://github.com/apache/drill/pull/2599#discussion_r1103724674


##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillAddAggForExceptRule.java:
##
@@ -0,0 +1,76 @@
+/*
+ * 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 org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelOptRuleOperand;
+import org.apache.calcite.plan.hep.HepRelVertex;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.trace.CalciteTrace;
+import org.apache.curator.shaded.com.google.common.collect.ImmutableList;
+import org.apache.drill.exec.planner.physical.PrelUtil;
+import org.slf4j.Logger;
+
+import static org.apache.drill.exec.ExecConstants.EXCEPT_ADD_AGG_BELOW;
+
+/**
+ * Rule that try to add agg for Except set op.
+ */
+public class DrillAddAggForExceptRule extends RelOptRule {
+  public static final RelOptRule INSTANCE = new 
DrillAddAggForExceptRule(RelOptHelper.any(DrillExceptRel.class), 
"DrillAddAggForExceptRule");
+  protected static final Logger tracer = CalciteTrace.getPlannerTracer();
+
+  public DrillAddAggForExceptRule(RelOptRuleOperand operand, String 
description) {
+super(operand, description);
+  }
+
+  @Override
+  public boolean matches(RelOptRuleCall call) {
+DrillExceptRel drillExceptRel = call.rel(0);
+return !drillExceptRel.all && !drillExceptRel.isAggAdded() && 
!findAggRel(drillExceptRel.getInput(0));
+  }
+
+  private boolean findAggRel(RelNode relNode) {
+if (relNode instanceof HepRelVertex) {
+  return findAggRel(((HepRelVertex) relNode).getCurrentRel());
+}
+if (relNode instanceof DrillAggregateRel) {
+  return true;
+}
+if (relNode.getInputs().size() == 1 && relNode.getInput(0) != null) {
+  return findAggRel(relNode.getInput(0));
+}
+return false;
+  }
+
+  @Override
+  public void onMatch(RelOptRuleCall call) {
+final DrillExceptRel drillExceptRel = call.rel(0);
+boolean addAggBelow = 
PrelUtil.getPlannerSettings(call.getPlanner()).getOptions().getOption(EXCEPT_ADD_AGG_BELOW);
+if (addAggBelow) {
+  RelNode aggNode = new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.getInput(0),
+ImmutableBitSet.range(0, 
drillExceptRel.getInput(0).getRowType().getFieldList().size()), 
ImmutableList.of(), ImmutableList.of());
+  call.transformTo(drillExceptRel.copy(ImmutableList.of(aggNode, 
drillExceptRel.getInput(1)), true));
+} else {
+  call.transformTo(new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.copy(true),

Review Comment:
   > Drill doesn't use streaming aggregate for distinct calls, so only hash agg 
should be enough.
   
   I see it checks aggregate.containsDistinctCall() in StreamAggPrule, but It 
will generate steam agg for sql like "select a,b,c from foo group by a,b,c".



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] Leon-WTF commented on a diff in pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

Leon-WTF commented on code in PR #2599:
URL: https://github.com/apache/drill/pull/2599#discussion_r1103724674


##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillAddAggForExceptRule.java:
##
@@ -0,0 +1,76 @@
+/*
+ * 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 org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelOptRuleOperand;
+import org.apache.calcite.plan.hep.HepRelVertex;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.trace.CalciteTrace;
+import org.apache.curator.shaded.com.google.common.collect.ImmutableList;
+import org.apache.drill.exec.planner.physical.PrelUtil;
+import org.slf4j.Logger;
+
+import static org.apache.drill.exec.ExecConstants.EXCEPT_ADD_AGG_BELOW;
+
+/**
+ * Rule that try to add agg for Except set op.
+ */
+public class DrillAddAggForExceptRule extends RelOptRule {
+  public static final RelOptRule INSTANCE = new 
DrillAddAggForExceptRule(RelOptHelper.any(DrillExceptRel.class), 
"DrillAddAggForExceptRule");
+  protected static final Logger tracer = CalciteTrace.getPlannerTracer();
+
+  public DrillAddAggForExceptRule(RelOptRuleOperand operand, String 
description) {
+super(operand, description);
+  }
+
+  @Override
+  public boolean matches(RelOptRuleCall call) {
+DrillExceptRel drillExceptRel = call.rel(0);
+return !drillExceptRel.all && !drillExceptRel.isAggAdded() && 
!findAggRel(drillExceptRel.getInput(0));
+  }
+
+  private boolean findAggRel(RelNode relNode) {
+if (relNode instanceof HepRelVertex) {
+  return findAggRel(((HepRelVertex) relNode).getCurrentRel());
+}
+if (relNode instanceof DrillAggregateRel) {
+  return true;
+}
+if (relNode.getInputs().size() == 1 && relNode.getInput(0) != null) {
+  return findAggRel(relNode.getInput(0));
+}
+return false;
+  }
+
+  @Override
+  public void onMatch(RelOptRuleCall call) {
+final DrillExceptRel drillExceptRel = call.rel(0);
+boolean addAggBelow = 
PrelUtil.getPlannerSettings(call.getPlanner()).getOptions().getOption(EXCEPT_ADD_AGG_BELOW);
+if (addAggBelow) {
+  RelNode aggNode = new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.getInput(0),
+ImmutableBitSet.range(0, 
drillExceptRel.getInput(0).getRowType().getFieldList().size()), 
ImmutableList.of(), ImmutableList.of());
+  call.transformTo(drillExceptRel.copy(ImmutableList.of(aggNode, 
drillExceptRel.getInput(1)), true));
+} else {
+  call.transformTo(new DrillAggregateRel(drillExceptRel.getCluster(), 
drillExceptRel.getTraitSet(), drillExceptRel.copy(true),

Review Comment:
   > Drill doesn't use streaming aggregate for distinct calls, so only hash agg 
should be enough.
   
   @vvysotskyi I see it checks aggregate.containsDistinctCall() in 
StreamAggPrule, but It will generate steam agg for sql like "select a,b,c from 
foo group by a,b,c".



-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre merged pull request #2755: DRILL-8399: MS Access Reader Misinterprets Data Types

[via GitHub]

cgivre merged PR #2755:
URL: https://github.com/apache/drill/pull/2755


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre commented on pull request #2657: DRILL-8316: Convert Druid Storage Plugin to EVF & V2 JSON Reader

[via GitHub]

cgivre commented on PR #2657:
URL: https://github.com/apache/drill/pull/2657#issuecomment-1427088906

   @Z0ltrix Would you mind reviewing this?   The unit tests pass, but I'm not 
100% certain this is handling large data sets correctly.


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre commented on pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

cgivre commented on PR #2599:
URL: https://github.com/apache/drill/pull/2599#issuecomment-1427120271

   @Leon-WTF Thanks for this.   @vvysotskyi Thanks for the review.  Merging now!


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre merged pull request #2599: DRILL-4232: Support for EXCEPT and INTERSECT set operator

[via GitHub]

cgivre merged PR #2599:
URL: https://github.com/apache/drill/pull/2599


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton merged pull request #2754: DRILL-8398: Fix GitHub Actions to use proper JDK version

[via GitHub]

jnturton merged PR #2754:
URL: https://github.com/apache/drill/pull/2754


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton commented on pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

jnturton commented on PR #2499:
URL: https://github.com/apache/drill/pull/2499#issuecomment-1427686523

   I think this one is licked! I've requested a review from @cgivre to get it a 
once over from a pair of eyes belonging to a non-author.


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton commented on pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

jnturton commented on PR #2499:
URL: https://github.com/apache/drill/pull/2499#issuecomment-1427809955

   Let me see what's gone awry with the Hive tests...


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre commented on pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

cgivre commented on PR #2499:
URL: https://github.com/apache/drill/pull/2499#issuecomment-1427881798

   > Let me see what's gone awry with the Hive tests...
   @jnturton Did you do a rebase with DRILL-8398 and DRILL-8400?
   
   


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton commented on pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

jnturton commented on PR #2499:
URL: https://github.com/apache/drill/pull/2499#issuecomment-1427913119

   > > @jnturton Did you do a rebase with DRILL-8398 and DRILL-8400?
   
   I did think of that and now have, 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.

To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] LYCJeff commented on pull request #2747: DRILL-8393: Allow parameters to be passed to headers through SQL in WHERE clause

[via GitHub]

LYCJeff commented on PR #2747:
URL: https://github.com/apache/drill/pull/2747#issuecomment-1428198156

   @cgivre I'm not sure if this change is appropriate. I'd love to hear your 
opinion.


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton commented on pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

jnturton commented on PR #2499:
URL: https://github.com/apache/drill/pull/2499#issuecomment-1428211191

   The Hive unit tests are quite problematic for me locally due to more 
problems than you'd expect when I try to do test runs under JDK 8. Anyway, I 
think that I've fixed the two broken Hive impersonation tests and rebased on 
master in [this new PR to your 
branch](https://github.com/kingswanwho/drill/pull/3).


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] kingswanwho commented on pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

kingswanwho commented on PR #2499:
URL: https://github.com/apache/drill/pull/2499#issuecomment-1428239825

   > The Hive unit tests are quite problematic for me locally due to more 
problems than you'd expect when I try to do test runs under JDK 8. Anyway, I 
think that I've fixed the two broken Hive impersonation tests and rebased on 
master in [this new PR to your 
branch](https://github.com/kingswanwho/drill/pull/3).
   
   I have merged and pushed, it should work well this time : -)


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre commented on pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

cgivre commented on PR #2499:
URL: https://github.com/apache/drill/pull/2499#issuecomment-1428284278

   @jnturton @kingswanwho I'll review once CI passes later today.  Looks good 
so far!


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre commented on pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

cgivre commented on PR #2499:
URL: https://github.com/apache/drill/pull/2499#issuecomment-1428502853

   @kingswanwho Looks like we're still getting some errors on the Hive unit 
tests.  I'm rerunning to see if it was a fluke or not.


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre commented on pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

cgivre commented on PR #2499:
URL: https://github.com/apache/drill/pull/2499#issuecomment-1428975963

   @kingswanwho @jnturton It looks like Java 8 is still not liking the Hive 
unit tests.  


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] kingswanwho commented on pull request #2499: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

kingswanwho commented on PR #2499:
URL: https://github.com/apache/drill/pull/2499#issuecomment-1428986001

   > Details
   
   Hi Charles, Thanks for the remind! Need James' help to investigate.


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] jnturton opened a new pull request, #2756: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

jnturton opened a new pull request, #2756:
URL: https://github.com/apache/drill/pull/2756

   # [DRILL-8117](https://issues.apache.org/jira/browse/DRILL-8117): Upgrade 
unit tests to the cluster fixture framework
   
   ## Description
   
   Upgrades various unit tests to the cluster fixture framework and replaces 
other instances of deprecated code usage.
   
   Methods client.alterSession() and client.alterSystem() run sql silently 
which would not throw exception, so it doesn't match the purpose for some test 
cases. And I still keep using run() here for consistence.
   
   For `TestInboundImpersonation`
   Because cluster fixture save client properties even client closed, use a 
dedicated cluster fixture for each test case to ensure the client fixture has 
clean properties.
   
   For `TestImpersonationMetadata`
   Only use one cluster from test start to the end will induce buffer overflow. 
So change BeforeClass to Before to start a new cluster for each test case.
   
   ## Documentation
   
   N/A
   
   ## Testing
   
   Existing unit tests.
   


-- 
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: dev-unsubscr...@drill.apache.org

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

[GitHub] [drill] cgivre commented on pull request #2756: DRILL-8117: Upgrade unit tests to the cluster fixture framework

[via GitHub]

cgivre commented on PR #2756:
URL: https://github.com/apache/drill/pull/2756#issuecomment-1429699470

   @jnturton @kingswanwho Should we close the other 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.

To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org

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