DRILL-6834: Introduce option to disable result set on CTAS, create vi… #1549
Conversation
There was a problem hiding this comment.
I have a question regarding the used approach. With the current implementation, updateCount is taken from the writer, or somewhere else and set to the hidden value vector, which is handled later. Is it possible to use some other mechanisms to specify updateCount, for example, use additional query option etc?
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java
Outdated
Show resolved
Hide resolved
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java
Outdated
Show resolved
Hide resolved
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java
Outdated
Show resolved
Hide resolved
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContextImpl.java
Outdated
Show resolved
Hide resolved
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/QueryOptionManager.java
Outdated
Show resolved
Hide resolved
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SessionOptionManager.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/drill/exec/physical/impl/materialize/VectorRecordMaterializer.java
Outdated
Show resolved
Hide resolved
| if (qrb.getHeader().hasUpdateCount()) { | ||
| int updateCount = qrb.getHeader().getUpdateCount(); | ||
| int currentUpdateCount = statement.getUpdateCount() == -1 ? 0 : statement.getUpdateCount(); | ||
| ((DrillStatement) statement).setUpdateCount(updateCount + currentUpdateCount); |
There was a problem hiding this comment.
Is it possible to set updateCount and other fields which are changed there during statement creation? It may allow us to avoid adding additional setters to the DrillStatement interface.
There was a problem hiding this comment.
Unfortunately not, because ResultSet is set before actual data is queried (AvaticaConnection).
However, DrillStatement#setResultSet(AvaticaResultSet) may be removed and openResultSet (see DrillStatementImpl below) can be nullified when update count is set with #setUpdateCount(int) method.
There was a problem hiding this comment.
@vvysotskyi, I've updated the PR, please take a look.
Regarding your question, I do not see any other mechanism (yet) for passing affected row (update) count. Passing it as an option is not a good idea either, because the updateCount is actually a query result.
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java
Outdated
Show resolved
Hide resolved
| if (qrb.getHeader().hasUpdateCount()) { | ||
| int updateCount = qrb.getHeader().getUpdateCount(); | ||
| int currentUpdateCount = statement.getUpdateCount() == -1 ? 0 : statement.getUpdateCount(); | ||
| ((DrillStatement) statement).setUpdateCount(updateCount + currentUpdateCount); |
There was a problem hiding this comment.
Unfortunately not, because ResultSet is set before actual data is queried (AvaticaConnection).
However, DrillStatement#setResultSet(AvaticaResultSet) may be removed and openResultSet (see DrillStatementImpl below) can be nullified when update count is set with #setUpdateCount(int) method.
KazydubB
force-pushed
the
DRILL-6834
branch
5 times, most recently
from
fac1d40
to
d463641
Compare
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java
Outdated
Show resolved
Hide resolved
| @@ -34,7 +34,15 @@ public FragmentOptionManager(OptionManager systemOptions, OptionList options) { | |||
|
|
|||
| private static Map<String, OptionValue> getMapFromOptionList(final OptionList options) { | |||
| final Map<String, OptionValue> tmp = Maps.newHashMap(); | |||
There was a problem hiding this comment.
Could you please replace Maps.newHashMap() with constructor
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/WriterRecordBatch.java
Outdated
Show resolved
Hide resolved
| for (final OptionValue option : options) { | ||
| OptionValue value = tmp.get(option.name); | ||
| if (value != null) { | ||
| // As there may be the same option defined for different scopes |
There was a problem hiding this comment.
Could you please explain cases, when the same option defined for different scopes will be present in options list. Should this logic be added to other option managers?
Is it possible to construct options list considering options precedence to avoid adding the same logic for other option managers?
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
Outdated
Show resolved
Hide resolved
|
|
||
| assertFalse("SET query should not return result set", hasResult); | ||
| assertNull("No result", s.getResultSet()); | ||
| assertNotEquals("Update count should be >= 0", -1, s.getUpdateCount()); |
There was a problem hiding this comment.
Not sure that we should also verify updateCount value. It is required to check that there is no result set.
exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestJdbcQuery.java
Outdated
Show resolved
Hide resolved
exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestJdbcQuery.java
Outdated
Show resolved
Hide resolved
|
@vvysotskyi, thanks for your review. I've updated the PR |
…r JDBC connection - Added session-scoped option `drill.exec.fetch_resultset_for_ddl` to control whether update count or result set should be returned for JDBC connection session. By default the option is set to `true` which ensures that result set is returned; - Updated Drill JDBC: `DrillCursor` and `DrillStatement` to achieve desired behaviour.
…r JDBC connection - Added session-scoped option `drill.exec.fetch_resultset_for_ddl` to control whether update count or result set should be returned for JDBC connection session. By default the option is set to `true` which ensures that result set is returned; - Updated Drill JDBC: `DrillCursor` and `DrillStatement` to achieve desired behaviour. closes apache#1549
…r JDBC connection - Added session-scoped option `drill.exec.fetch_resultset_for_ddl` to control whether update count or result set should be returned for JDBC connection session. By default the option is set to `true` which ensures that result set is returned; - Updated Drill JDBC: `DrillCursor` and `DrillStatement` to achieve desired behaviour. closes apache#1549
…r JDBC connection - Added session-scoped option `drill.exec.fetch_resultset_for_ddl` to control whether update count or result set should be returned for JDBC connection session. By default the option is set to `true` which ensures that result set is returned; - Updated Drill JDBC: `DrillCursor` and `DrillStatement` to achieve desired behaviour. closes apache#1549
…ew and drop table/view etc. for JDBC connection
drill.exec.fetch_resultset_for_ddlto control whether update count or result set should be returned for JDBC connection session. Eligible queries are CTAS, CREATE VIEW, CREATE FUNCTION, DROP TABLE, DROP VIEW, DROP FUNCTION, USE schema, SET option, REFRESH METADATA TABLE. By default the option is set totruewhich ensures that result set is returned;DrillCursorandDrillStatementto achieve desired behaviour;