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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
