jnturton commented on a change in pull request #2419:
URL: https://github.com/apache/drill/pull/2419#discussion_r789682495
##########
File path:
exec/java-exec/src/main/codegen/templates/ParquetOutputRecordWriter.java
##########
@@ -142,94 +142,110 @@ public void writeField() throws IOException {
minor.class == "Decimal9" ||
minor.class == "UInt4">
<#if mode.prefix == "Repeated" >
- reader.read(i, holder);
- consumer.addInteger(holder.value);
+ reader.read(i, holder);
+ consumer.addInteger(holder.value);
<#else>
- consumer.startField(fieldName, fieldId);
- reader.read(holder);
- consumer.addInteger(holder.value);
- consumer.endField(fieldName, fieldId);
+ consumer.startField(fieldName, fieldId);
+ reader.read(holder);
+ consumer.addInteger(holder.value);
+ consumer.endField(fieldName, fieldId);
</#if>
<#elseif
minor.class == "Float4">
<#if mode.prefix == "Repeated" >
- reader.read(i, holder);
- consumer.addFloat(holder.value);
+ reader.read(i, holder);
+ consumer.addFloat(holder.value);
<#else>
- consumer.startField(fieldName, fieldId);
- reader.read(holder);
- consumer.addFloat(holder.value);
- consumer.endField(fieldName, fieldId);
+ consumer.startField(fieldName, fieldId);
+ reader.read(holder);
+ consumer.addFloat(holder.value);
+ consumer.endField(fieldName, fieldId);
</#if>
<#elseif
minor.class == "BigInt" ||
minor.class == "Decimal18" ||
- minor.class == "TimeStamp" ||
minor.class == "UInt8">
<#if mode.prefix == "Repeated" >
- reader.read(i, holder);
- consumer.addLong(holder.value);
+ reader.read(i, holder);
+ consumer.addLong(holder.value);
<#else>
- consumer.startField(fieldName, fieldId);
- reader.read(holder);
- consumer.addLong(holder.value);
- consumer.endField(fieldName, fieldId);
+ consumer.startField(fieldName, fieldId);
+ reader.read(holder);
+ consumer.addLong(holder.value);
+ consumer.endField(fieldName, fieldId);
</#if>
+ <#elseif minor.class == "TimeStamp" >
+ <#if mode.prefix == "Repeated" >
+ reader.read(i, holder);
+ // Write Drill timestamp directly: writes local time to Parquet's UTC
type.
+ // This is a bug: see DRILL-8099.
+ // The commented-out line is the correct way to do this. However, doing
+ // the correct way breaks other Drill code as explained in DRILL-8099.
Review comment:
Okay so this case is yet to be fixed (in another PR).
##########
File path:
contrib/storage-jdbc/src/test/java/org/apache/drill/exec/store/jdbc/TestJdbcPluginWithH2IT.java
##########
@@ -100,6 +101,9 @@ public void testCrossSourceMultiFragmentJoin() throws
Exception {
}
@Test
+ // See Drill-8101: The timestamp type is broken: this test only works
+ // in UTC.
+ @Ignore("Only works in the UTC timezone")
Review comment:
While this shrinks the unit test coverage of our CI build, TIMESTAMP is
a well documented priority so I'm not worried that we will not return to these
tests.
##########
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/scan/RowBatchReader.java
##########
@@ -81,31 +81,28 @@
* from any method. A {@link UserException} is preferred to provide
* detailed information about the source of the problem.
*/
-
public interface RowBatchReader {
/**
* Name used when reporting errors. Can simply be the class name.
*
* @return display name for errors
*/
-
String name();
/**
* Setup the record reader. Called just before the first call
- * to <tt>next()</tt>. Allocate resources here, not in the constructor.
+ * to {@code next()}. Allocate resources here, not in the constructor.
Review comment:
@paul-rogers is this commentary still correct? In the HTTP log format
batch reader above, the new pattern is resource allocation in the constructor
and not in `open()`...
##########
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/scan/v3/lifecycle/OutputBatchBuilder.java
##########
@@ -277,11 +279,18 @@ protected void defineSourceBatchMapping(TupleMetadata
schema, int source) {
@SuppressWarnings("unchecked")
private void physicalProjection() {
outputContainer.removeAll();
+ mapVectors.clear();
for (int i = 0; i < outputSchema.size(); i++) {
- ValueVector outputVector;
ColumnMetadata outputCol = outputSchema.metadata(i);
+ ValueVector outputVector;
if (outputCol.isMap()) {
outputVector = buildTopMap(outputCol, (List<VectorSource>)
vectorSources[i]);
+
+ // Map vectors are a nuisance: they carry their own value could which
Review comment:
```suggestion
// Map vectors are a nuisance: they carry their own value which
```
##########
File path:
contrib/storage-jdbc/src/test/java/org/apache/drill/exec/store/jdbc/TestJdbcWriterWithH2.java
##########
@@ -61,7 +60,7 @@
public static void init() throws Exception {
startCluster(ClusterFixture.builder(dirTestWatcher));
// Force timezone to UTC for these tests.
Review comment:
Well that's one way to do it 😆.
##########
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/scan/v3/lifecycle/ReaderLifecycle.java
##########
@@ -226,10 +240,16 @@ public boolean next() {
// a new batch just to learn about EOF. Don't read if the reader
// already reported EOF. In that case, we're just processing any last
// lookahead row in the result set loader.
+ //
+ // If the scan has hit is pushed-down limit, then the reader might
Review comment:
```suggestion
// If the scan has hit its pushed-down limit, then the reader might
```
--
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]
