drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [drill] paul-rogers commented on a change in pull request #2290: DRILL-7984: Support clickhouse by JDBC plugin
Date Mon, 09 Aug 2021 21:47:09 GMT

paul-rogers commented on a change in pull request #2290:
URL: https://github.com/apache/drill/pull/2290#discussion_r685538830



##########
File path: contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/JdbcPrel.java
##########
@@ -39,6 +39,9 @@
 import org.apache.drill.exec.planner.physical.visitor.PrelVisitor;
 import org.apache.drill.exec.record.BatchSchema.SelectionVectorMode;
 import org.apache.drill.exec.store.SubsetRemover;
+import org.apache.drill.exec.store.jdbc.clickhouse.ClickhouseConstant;
+import org.apache.drill.exec.store.jdbc.clickhouse.ClickhouseDialect;
+import org.apache.drill.exec.store.jdbc.clickhouse.ClickhouseJdbcImplementor;

Review comment:
       See comment above. The generic `JdbcPrel` is not the place to add vendor-specific code.
However, this class can call a vendor-specific helper.

##########
File path: contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/clickhouse/ClickhouseConstant.java
##########
@@ -0,0 +1,27 @@
+/*
+ * 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.store.jdbc.clickhouse;
+
+/**
+ * @author feiteng.wtf
+ * @date 2021-08-08
+ */

Review comment:
       Generally Drill does not include the author and date. What we do include is an explanation
of what the class is about.

##########
File path: contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/clickhouse/ClickhouseDialect.java
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.store.jdbc.clickhouse;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.sql.SqlBasicTypeNameSpec;
+import org.apache.calcite.sql.SqlDataTypeSpec;
+import org.apache.calcite.sql.SqlDialect;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlUserDefinedTypeNameSpec;
+import org.apache.calcite.sql.SqlWriter;
+import org.apache.calcite.sql.parser.SqlParserPos;
+import org.apache.calcite.sql.type.BasicSqlType;
+
+/**
+ * @author feiteng.wtf
+ * @date 2021-08-08
+ */

Review comment:
       See comment above.

##########
File path: contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/writers/JdbcBigintWriter.java
##########
@@ -31,9 +31,9 @@ public JdbcBigintWriter(String colName, RowSetLoader rowWriter, int columnIndex)
 
   @Override
   public void load(ResultSet results) throws SQLException {
-    boolean b = results.wasNull();
-    if (! results.wasNull()) {
-      long value = results.getLong(columnIndex);
+    // clickhouse requires getting the column before checking nullability
+    long value = results.getLong(columnIndex);
+    if (!results.wasNull()) {

Review comment:
       Good catch! As it turns out, the original code is wrong for all DBs, not just Clickhouse.
See [this article](https://schneide.blog/2021/02/22/jdbcs-wasnull-method-pitfall/).
   
   Suggestion, change the comment to something like "JDBC reports nullability only after getting
the column value." Here and below.

##########
File path: contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/JdbcCatalogSchema.java
##########
@@ -53,11 +54,12 @@
     try (Connection con = source.getConnection();
          ResultSet set = con.getMetaData().getCatalogs()) {
       connectionSchemaName = con.getSchema();
-      while (set.next()) {
-        final String catalogName = set.getString(1);
-        CapitalizingJdbcSchema schema = new CapitalizingJdbcSchema(
-            getSchemaPath(), catalogName, source, dialect, convention, catalogName, null,
caseSensitive);
-        schemaMap.put(schema.getName(), schema);
+      if (!ClickhouseConstant.PRODUCT_NAME.equals(con.getMetaData().getDatabaseProductName()))
{

Review comment:
       Let's think about this approach a bit. There is no comment here (there should be),
so I'll infer that Clickhouse somehow does something special. Cool. So I'm working on Postgres,
or DB X and I want to do something similar. Do we end up with a bit set of if-statements?
If I work on DB X, how do I ensure I don't break Clickhouse, since I can't run that code?
   
   Can we think about a way of abstracting this kind of thing into a DB-specific helper class?
The default helper does "stock" JDBC. Clickhouse (and my DB X) code go into DB-specific helpers.
Now, to get DB X to work, I don't have to worry about breaking Clickhouse.
   
   See if you can work out a "mini-plugin" for such code. Happy to provide suggestions if
you like.

##########
File path: contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/clickhouse/ClickhouseJdbcImplementor.java
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.store.jdbc.clickhouse;
+
+import org.apache.drill.shaded.guava.com.google.common.base.Preconditions;
+import org.apache.drill.shaded.guava.com.google.common.collect.ImmutableList;
+import org.apache.calcite.adapter.java.JavaTypeFactory;
+import org.apache.calcite.adapter.jdbc.JdbcImplementor;
+import org.apache.calcite.adapter.jdbc.JdbcTableScan;
+import org.apache.calcite.sql.SqlDialect;
+import org.apache.calcite.sql.SqlIdentifier;
+
+import java.util.Iterator;
+
+/**
+ * @author feiteng.wtf
+ * @date 2021-07-26
+ */

Review comment:
       Ditto.

##########
File path: contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/clickhouse/ClickhouseDialect.java
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.store.jdbc.clickhouse;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.sql.SqlBasicTypeNameSpec;
+import org.apache.calcite.sql.SqlDataTypeSpec;
+import org.apache.calcite.sql.SqlDialect;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlUserDefinedTypeNameSpec;
+import org.apache.calcite.sql.SqlWriter;
+import org.apache.calcite.sql.parser.SqlParserPos;
+import org.apache.calcite.sql.type.BasicSqlType;
+
+/**
+ * @author feiteng.wtf
+ * @date 2021-08-08
+ */
+public class ClickhouseDialect extends SqlDialect {

Review comment:
       This is a great example of a vendor-specific implementation. Can we follow the same
pattern elsewhere?

##########
File path: contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/JdbcCatalogSchema.java
##########
@@ -108,7 +113,7 @@ private boolean addSchemas(DataSource source, SqlDialect dialect, DrillJdbcConve
 
         String parentKey = StringUtils.lowerCase(catalogName);
         CapitalizingJdbcSchema parentSchema = schemaMap.get(parentKey);
-        if (parentSchema == null) {
+        if (parentSchema == null || isFirstLevel) {

Review comment:
       Nit: comment to explain why the first level matters.




-- 
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-unsubscribe@drill.apache.org

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



Mime
View raw message