-
Notifications
You must be signed in to change notification settings - Fork 982
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DRILL-8005: Add Writer to JDBC Storage Plugin #2327
Conversation
This pull request introduces 2 alerts when merging 57d3fa6 into bad5e66 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging af78c5a into bad5e66 - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments and questions inline.
contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/utils/JdbcQueryBuilder.java
Show resolved
Hide resolved
String cleanSQL = node.toSqlString(dialect).getSql(); | ||
|
||
// TODO Fix this hack | ||
// HACK See CALCITE-4820 (https://issues.apache.org/jira/browse/CALCITE-4820) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you see the response from the Calcite team on CALCITE-4820?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see the response.
@vvysotskyi would it be possible to merge apache/calcite#1568 to the Drill calcite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think yes, it is a quite small fix, so no conflicts should appear.
contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/CapitalizingJdbcSchema.java
Outdated
Show resolved
Hide resolved
contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/JdbcCatalogSchema.java
Outdated
Show resolved
Hide resolved
.../storage-jdbc/src/test/java/org/apache/drill/exec/store/jdbc/TestJdbcPluginWithPostgres.java
Show resolved
Hide resolved
return new NullableIntJDBCConverter(fieldId, fieldName, reader, fields); | ||
} | ||
|
||
public class NullableIntJDBCConverter extends FieldConverter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess maybe we could do something with a Freemarker template for the converters but I'm not convinced it's worth it now that we already have these written.
contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/JdbcRecordWriter.java
Show resolved
Hide resolved
} | ||
} | ||
|
||
private String buildInsertQuery() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the maximum number of records DBMSes allow in a VALUES
expression is commonly order 1e3 to 1e4. If Drill batch sizes can exceed that we're going to have a problem. A possible solution is to always partition into conservative insert batches of, say 500 records. The PreparedStatement
and executeBatch
JDBC API usage in this answer https://stackoverflow.com/a/3786127/1153953 might help to keep things as efficient as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cgivre did you see this? Have we tested CTAS statements with 10k, 100k, 1m records?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dzamo
This is a good question. What is supposed to happen is that inserts actually happen in batches. Any suggestions as to how to test? Do you think I should just generate a CSV file with 1M records and see what happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still learning about the writer API myself, so I'm figuring this out as we go, but I'm also not quite sure where you control the batch size. I can see if I can figure that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cgivre I think generating a test file of 1m records is a good thing to do at least once. I don't know much about Drill's batching but I think of it as unrelated to the size limitations of VALUES expressions in external dbs. If it were me I'd assume Drill could send batches bigger than the target db's VALUES limit and I'd write a loop in JdbcRecordWriter that inserts no more than ~500 records at a time, as outlined in my first comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cgivre a little trick for producing 1e6 records
set `planner.enable_nljoin_for_scalar_only` = false;
create temporary table t as select o1.* from cp.`tpch/orders.parquet` o1 cross join cp.`tpch/orders.parquet` o2 limit 1e6;
contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/CapitalizingJdbcSchema.java
Outdated
Show resolved
Hide resolved
contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/JdbcStoragePlugin.java
Outdated
Show resolved
Hide resolved
@dzamo, @vvysotskyi |
@cgivre Hello Charles. Is it possible to compress the |
Or we can use a cross join in a query based on a small CSV file of n rows to get n² rows. |
Hi @luocooong Are you looking for a test to see if it can go from compressed file to insert? I added a new test that generates a 100k row CSV file and inserts that. The only thing is that the test is slow, so I disabled it by default. |
contrib/storage-jdbc/src/test/java/org/apache/drill/exec/store/jdbc/TestJdbcWriterWithH2.java
Show resolved
Hide resolved
.../storage-jdbc/src/test/java/org/apache/drill/exec/store/jdbc/TestJdbcWriterWithPostgres.java
Show resolved
Hide resolved
contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/JdbcRecordWriter.java
Outdated
Show resolved
Hide resolved
Statement stmt = connection.createStatement(); | ||
stmt.execute(insertQuery); | ||
logger.debug("Query complete"); | ||
// Close connection | ||
AutoCloseables.closeSilently(stmt, connection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please wrap the statement into the try-with-resources block and add a closing connection to the finally
block, since, for the case of exception during statement execution, it wouldn't be closed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed I think.
public void endRecord() throws IOException { | ||
logger.debug("Ending record"); | ||
|
||
// Add values to rowString |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use Calcite's JdbcTableModify
to create insert statement string instead of using custom logic?
contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/JdbcRecordWriter.java
Show resolved
Hide resolved
|
||
<dependency> | ||
<groupId>com.github.vvysotskyi.drill-calcite</groupId> | ||
<artifactId>calcite-server</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain why this dependency is required here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calcite-core is included in the root pom.xml
file. The DDR operations which we needed for this are in the calcite-server
module which was not listed as a dependency in the root pom.xml
.
@dzamo @vvysotskyi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Vova's happy I'm happy +1.
} | ||
} | ||
|
||
private String buildInsertQuery() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cgivre I think generating a test file of 1m records is a good thing to do at least once. I don't know much about Drill's batching but I think of it as unrelated to the size limitations of VALUES expressions in external dbs. If it were me I'd assume Drill could send batches bigger than the target db's VALUES limit and I'd write a loop in JdbcRecordWriter that inserts no more than ~500 records at a time, as outlined in my first comment.
} | ||
} | ||
|
||
private String buildInsertQuery() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cgivre a little trick for producing 1e6 records
set `planner.enable_nljoin_for_scalar_only` = false;
create temporary table t as select o1.* from cp.`tpch/orders.parquet` o1 cross join cp.`tpch/orders.parquet` o2 limit 1e6;
@dzamo The unit tests pass and I ran this locally with a 1M row CSV insert into a MySQL database which worked perfectly. Previously, this ran into the |
@cgivre this is great. I thought of one more possible optimisation: creating a parameterised INSERT PreparedStatement of writer_batch_size rows and reusing it for as long as there are >= writer_batch_size rows remaining to insert. I don't know Calcite stuff but I can say I saw a class called SqlDynamicParam in it. This would mean that the receiving DBMS does not need to parse a very long INSERT statement at the start of every batch, a noticeable saving of memory and CPU time for it I would guess. Just a possible optimisation I wanted to share, I view it as something that can also come in a later version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Thank you @dzamo @vvysotskyi and @luocooong for your review! |
* Initial commit * Getting to writer * Schema should be created * Queries complete * Queries successfully completing * WIP * WIP * Fixed schema resolution issue * Fixed VARCHAR precision issue * Added writable config option * Mostly working * Added Postgres Unit Tests, Some working * All Postgres tests working * Fix spacing * Code Cleanup * WIP * WIP - Null rows * Null input working * Added additional unit tests * Finished MySQL unit tests * MySQL and Postgres Tests all Passing * h2 writer tests working * Final commit * Ready for PR * Final fixes * Add license header * Fixed MySQL Timezones in unit tests * Removed unused import * Remove unneeded unit tests * Minor fixes * Working on docs * Addressed code comments * Fixed unit test * Added test for large file * Updated tutorial * Added tests for large inserts * Added documentation * Addressed Review Commnet * De-hacking * Removed ROW Hack * Added configurable batching
DRILL-8005: Add Writer to JDBC Storage Plugin
Description
This PR adds the ability to write to JDBC storage. Users will be able to execute the following queries against JDBC data sources.
CREATE TABLE AS
CREATE TABLE IF NOT EXISTS
DROP TABLE
Example Queries:
Known Limitations:
NUMERIC
for mostFLOAT
types.VARBINARY
is not supported yet.Documentation
Documentation will be provided in a separate pull request.
Testing
This PR adds unit tests for the writer for MySQL, Postgres, and H2. Additionally, this PR adds additional unit tests for the JDBC storage plugin and Postgres.