Skip to content

ARROW-1816: [Java] Resolve new vector classes structure for timestamp, date and maybe interval #1330

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.

Sign up for GitHub

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

Closed
icexelloss wants to merge 2 commits into from
Closed

ARROW-1816: [Java] Resolve new vector classes structure for timestamp, date and maybe interval #1330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e79792a
Init commit
icexelloss Nov 18, 2017
6c33285
Clean up
icexelloss Nov 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions java/vector/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,13 @@
<groupId>org.apache.drill.tools</groupId>
<artifactId>drill-fmpp-maven-plugin</artifactId>
<version>1.5.0</version>
<dependencies>
<dependency>
<groupId>org.freemarker</groupId>
<artifactId>freemarker</artifactId>
<version>2.3.23</version>
</dependency>
</dependencies>
Copy link
Member

@BryanCutler BryanCutler Nov 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come this needs to be added now? If it does can it be scoped to compile phase?

Copy link
Contributor Author

@icexelloss icexelloss Nov 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should just be build dependency. Before it was using 2.3.21.

Copy link
Member

@BryanCutler BryanCutler Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So without specifying the version it defaults to 2.3.21? Was that version causing an issue?

Copy link
Contributor Author

@icexelloss icexelloss Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think that's the version comes with the plugin.

I used <#sep> notation in the template to created comma separated arg list. The notation is introduced in 2.3.23

<executions>
<execution>
<id>generate-fmpp</id>
Expand Down
26 changes: 5 additions & 21 deletions java/vector/src/main/codegen/data/ValueVectorTypes.tdd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,26 +73,10 @@
{ class: "UInt8" },
{ class: "Float8", javaType: "double", boxedType: "Double", fields: [{name: "value", type: "double"}] },
{ class: "DateMilli", javaType: "long", friendlyType: "LocalDateTime" },
{ class: "TimeStampSec", javaType: "long", boxedType: "Long", friendlyType: "LocalDateTime" },
{ class: "TimeStampMilli", javaType: "long", boxedType: "Long", friendlyType: "LocalDateTime" },
{ class: "TimeStampMicro", javaType: "long", boxedType: "Long", friendlyType: "LocalDateTime" },
{ class: "TimeStampNano", javaType: "long", boxedType: "Long", friendlyType: "LocalDateTime" },
{ class: "TimeStampSecTZ", javaType: "long", boxedType: "Long",
typeParams: [ {name: "timezone", type: "String"} ],
arrowType: "org.apache.arrow.vector.types.pojo.ArrowType.Timestamp",
arrowTypeConstructorParams: ["org.apache.arrow.vector.types.TimeUnit.SECOND", "timezone"] },
{ class: "TimeStampMilliTZ", javaType: "long", boxedType: "Long",
typeParams: [ {name: "timezone", type: "String"} ],
arrowType: "org.apache.arrow.vector.types.pojo.ArrowType.Timestamp",
arrowTypeConstructorParams: ["org.apache.arrow.vector.types.TimeUnit.MILLISECOND", "timezone"] },
{ class: "TimeStampMicroTZ", javaType: "long", boxedType: "Long",
typeParams: [ {name: "timezone", type: "String"} ],
arrowType: "org.apache.arrow.vector.types.pojo.ArrowType.Timestamp",
arrowTypeConstructorParams: ["org.apache.arrow.vector.types.TimeUnit.MICROSECOND", "timezone"] },
{ class: "TimeStampNanoTZ", javaType: "long", boxedType: "Long",
typeParams: [ {name: "timezone", type: "String"} ],
arrowType: "org.apache.arrow.vector.types.pojo.ArrowType.Timestamp",
arrowTypeConstructorParams: ["org.apache.arrow.vector.types.TimeUnit.NANOSECOND", "timezone"] },
{ class: "Timestamp", javaType: "long", boxedType: "Long", friendlyType: "LocalDateTime"
Copy link
Member

@BryanCutler BryanCutler Nov 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the friendlyType param still used any more? If so then will LocalDateTime work with a timezone set?

Copy link
Contributor Author

@icexelloss icexelloss Nov 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used. The way it currently works is if timezone is specified, if will return the LocalDateTime in the specified time zone.

For example:

value = 0, timezone = null

returns LocalDateTime(1970-01-01 00:00:00)

value = 0, timezone = "America/New_York"

returns LocalDateTime(1969-12-31 19:00:00)

typeParams: [ {name: "unit", type: "TimeUnit"}, { name: "timezone", type: "String"} ],
arrowType: "org.apache.arrow.vector.types.pojo.ArrowType.Timestamp",
},
{ class: "TimeMicro" },
{ class: "TimeNano" }
]
Expand All @@ -116,7 +100,7 @@
{
class: "Decimal",
maxPrecisionDigits: 38, nDecimalDigits: 4, friendlyType: "BigDecimal",
typeParams: [ {name: "scale", type: "int"}, { name: "precision", type: "int"}],
typeParams: [ { name: "precision", type: "int"}, {name: "scale", type: "int"} ],
Copy link
Member

@BryanCutler BryanCutler Nov 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were previously flipped in ARROW-1091 and then flipped again in ARROW-1092. I thought they were right already, are they not?

Copy link
Contributor Author

@icexelloss icexelloss Nov 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reverse ordering the these typeParams are driving me nuts... I changed it such that they follow the same order everywhere. Also see: https://github.com/apache/arrow/pull/1330/files#r152436539

The end results are the same because flipped twice..

arrowType: "org.apache.arrow.vector.types.pojo.ArrowType.Decimal",
fields: [{name: "start", type: "int"}, {name: "buffer", type: "ArrowBuf"}]
}
Expand Down
1 change: 1 addition & 0 deletions java/vector/src/main/codegen/includes/vv_imports.ftl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ import java.math.BigDecimal;
import java.math.BigInteger;

import org.joda.time.DateTime;
import org.joda.time.DateTimeZone;
import org.joda.time.LocalDateTime;
import org.joda.time.Period;

Expand Down
7 changes: 7 additions & 0 deletions java/vector/src/main/codegen/templates/AbstractFieldWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,13 @@ public ListWriter list(String name) {
fail("${capName}(" + <#list minor.typeParams as typeParam>"${typeParam.name}: " + ${typeParam.name} + ", " + </#list>")");
return null;
}

@Override
public ${capName}Writer ${lowerName}(<#list minor.typeParams as typeParam>${typeParam.type} ${typeParam.name}<#sep>, </#list>) {
fail("${capName}(" + <#list minor.typeParams as typeParam>"${typeParam.name}: " + ${typeParam.name} + ", " + </#list>")");
return null;
}

</#if>

@Override
Expand Down
12 changes: 10 additions & 2 deletions java/vector/src/main/codegen/templates/AbstractPromotableFieldWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
* limitations under the License.
*/

import org.apache.arrow.vector.types.Types;
import org.apache.drill.common.types.TypeProtos.MinorType;

<@pp.dropOutputFile />
Expand All @@ -42,6 +43,8 @@ abstract class AbstractPromotableFieldWriter extends AbstractFieldWriter {
* @param type the type of the values we want to write
* @return the corresponding field writer
*/
abstract protected FieldWriter getWriter(ArrowType type);

abstract protected FieldWriter getWriter(MinorType type);
Copy link
Member

@BryanCutler BryanCutler Nov 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is is possible to just have getWriter(ArrowType)? Having 2 methods so similar is a little confusing.. This is because you need a writer for timestamp type params too?

Copy link
Contributor Author

@icexelloss icexelloss Nov 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree ideally we should have just one. However currently, this is needed to support this:

TimestampWriter timestampWriter = rootWriter.timestamp("a", TimeUnit.SECOND, "America/New_York");
timestampWriter.writeTimestamp(1000)

The second line here, the type of "timestampWriter" is a promotable writer and it needs to lookup the writer by MinorType.TIMESTAMP

We need to do more refactoring to support this API without abstract protected FieldWriter getWriter(MinorType type), but since this is going to be internal change and this PR is already quite complicated, I prefer this to be a follow up.

Copy link
Member

@BryanCutler BryanCutler Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sounds good


/**
Expand Down Expand Up @@ -118,7 +121,12 @@ public ListWriter list(String name) {
return getWriter(MinorType.MAP).${lowerName}(name<#list minor.typeParams as typeParam>, ${typeParam.name}</#list>);
}

</#if>
@Override
public ${capName}Writer ${lowerName}(<#list minor.typeParams as typeParam>${typeParam.type} ${typeParam.name}<#sep>, </#list>) {
return getWriter(MinorType.LIST).${lowerName}(<#list minor.typeParams as typeParam>${typeParam.name}<#sep>, </#list>);
}

<#else>
@Override
public ${capName}Writer ${lowerName}(String name) {
return getWriter(MinorType.MAP).${lowerName}(name);
Expand All @@ -128,7 +136,7 @@ public ListWriter list(String name) {
public ${capName}Writer ${lowerName}() {
return getWriter(MinorType.LIST).${lowerName}();
}

</#if>
</#list></#list>

public void copyReader(FieldReader reader) {
Expand Down
2 changes: 1 addition & 1 deletion java/vector/src/main/codegen/templates/ArrowType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ public static class ${name} extends <#if type.complex>ComplexType<#else>Primitiv

<#list fields as field>
<#assign fieldType = field.valueType!field.type>
${fieldType} ${field.name};
public ${fieldType} ${field.name};
</#list>

@JsonCreator
Expand Down
3 changes: 3 additions & 0 deletions java/vector/src/main/codegen/templates/BaseWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ public interface ListWriter extends BaseWriter {
<#if lowerName == "int" ><#assign lowerName = "integer" /></#if>
<#assign upperName = minor.class?upper_case />
<#assign capName = minor.class?cap_first />
<#if minor.typeParams?? >
${capName}Writer ${lowerName}(<#list minor.typeParams as typeParam>${typeParam.type} ${typeParam.name}<#sep>, </#list>);
</#if>
${capName}Writer ${lowerName}();
</#list></#list>
}
Expand Down
5 changes: 1 addition & 4 deletions java/vector/src/main/codegen/templates/ComplexReaders.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,7 @@ public void read(Nullable${minor.class?cap_first}Holder h){
return vector.getObject(idx());
}

<#if minor.class == "TimeStampSec" ||
minor.class == "TimeStampMilli" ||
minor.class == "TimeStampMicro" ||
minor.class == "TimeStampNano">
<#if minor.class == "Timestamp">
@Override
public ${minor.boxedType} read${minor.boxedType}(){
return vector.get(idx());
Expand Down
32 changes: 16 additions & 16 deletions java/vector/src/main/codegen/templates/MapWriters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,25 +231,25 @@ public void end() {
public ${minor.class}Writer ${lowerName}(String name) {
</#if>
FieldWriter writer = fields.get(handleCase(name));
FieldType fieldType = FieldType.nullable(
<#if minor.typeParams??>
<#if minor.arrowTypeConstructorParams??>
<#assign constructorParams = minor.arrowTypeConstructorParams />
<#else>
<#assign constructorParams = [] />
<#list minor.typeParams as typeParam>
<#assign constructorParams = constructorParams + [ typeParam.name ] />
</#list>
</#if>
new ${minor.arrowType}(${constructorParams?join(", ")})
<#else>
MinorType.${upperName}.getType()
</#if>);
if(writer == null) {
ValueVector vector;
ValueVector currentVector = container.getChild(name);
${vectName}Vector v = container.addOrGet(name,
FieldType.nullable(
<#if minor.typeParams??>
<#if minor.arrowTypeConstructorParams??>
<#assign constructorParams = minor.arrowTypeConstructorParams />
<#else>
<#assign constructorParams = [] />
<#list minor.typeParams?reverse as typeParam>
<#assign constructorParams = constructorParams + [ typeParam.name ] />
</#list>
</#if>
new ${minor.arrowType}(${constructorParams?join(", ")})
<#else>
MinorType.${upperName}.getType()
</#if>
),
fieldType,
${vectName}Vector.class);
writer = new PromotableWriter(v, container, getNullableMapWriterFactory());
vector = v;
Expand All @@ -264,7 +264,7 @@ public void end() {
} else {
if (writer instanceof PromotableWriter) {
// ensure writers are initialized
((PromotableWriter)writer).getWriter(MinorType.${upperName});
((PromotableWriter)writer).getWriter(fieldType.getType());
}
}
return writer;
Expand Down
22 changes: 19 additions & 3 deletions java/vector/src/main/codegen/templates/UnionListWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ public void setPosition(int index) {
<#assign uncappedName = name?uncap_first/>
<#if uncappedName == "int" ><#assign uncappedName = "integer" /></#if>
<#if !minor.typeParams?? >

@Override
public ${name}Writer ${uncappedName}() {
return this;
Expand All @@ -104,6 +103,24 @@ public void setPosition(int index) {
mapName = name;
return writer.${uncappedName}(name);
}

<#else>
@Override
public ${name}Writer ${uncappedName}() {
return this;
}

public ${name}Writer ${uncappedName}(<#list minor.typeParams as typeParam>${typeParam.type} ${typeParam.name}<#sep>, </#list>) {
ArrowType.${name} type = new ArrowType.${name}(<#list minor.typeParams as typeParam>${typeParam.name}<#sep>, </#list>);
writer.getWriter(type);
return this;
}

public ${name}Writer ${uncappedName}(String name, <#list minor.typeParams as typeParam>${typeParam.type} ${typeParam.name}<#sep>, </#list>) {
mapName = name;
return writer.${uncappedName}(name, <#list minor.typeParams as typeParam>${typeParam.name}<#sep>, </#list>);
}

</#if>
</#list></#list>

Expand Down Expand Up @@ -158,7 +175,7 @@ public void end() {
<#assign name = minor.class?cap_first />
<#assign fields = minor.fields!type.fields />
<#assign uncappedName = name?uncap_first/>
<#if !minor.typeParams?? >

@Override
public void write${name}(<#list fields as field>${field.type} ${field.name}<#if field_has_next>, </#if></#list>) {
writer.write${name}(<#list fields as field>${field.name}<#if field_has_next>, </#if></#list>);
Expand All @@ -170,7 +187,6 @@ public void write(${name}Holder holder) {
writer.setPosition(writer.idx()+1);
}

</#if>
</#list>
</#list>
}
4 changes: 0 additions & 4 deletions java/vector/src/main/codegen/templates/UnionReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,8 @@ private FieldReader getReaderForIndex(int index) {
<#list type.minor as minor>
<#assign name = minor.class?cap_first />
<#assign uncappedName = name?uncap_first/>
<#if !minor.typeParams?? >
case ${name?upper_case}:
return (FieldReader) get${name}();
</#if>
</#list>
</#list>
default:
Expand Down Expand Up @@ -158,7 +156,6 @@ public int size() {
<#assign friendlyType = (minor.friendlyType!minor.boxedType!type.boxedType) />
<#assign safeType=friendlyType />
<#if safeType=="byte[]"><#assign safeType="ByteArray" /></#if>
<#if !minor.typeParams?? >

private ${name}ReaderImpl ${uncappedName}Reader;

Expand All @@ -178,7 +175,6 @@ public void read(Nullable${name}Holder holder){
public void copyAsValue(${name}Writer writer){
getReaderForIndex(idx()).copyAsValue(writer);
}
</#if>
</#list>
</#list>

Expand Down
55 changes: 39 additions & 16 deletions java/vector/src/main/codegen/templates/UnionVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,16 +138,12 @@ public List<BufferBacked> getFieldInnerVectors() {
throw new UnsupportedOperationException("There are no inner vectors. Use geFieldBuffers");
}

private String fieldName(MinorType type) {
return type.name().toLowerCase();
private String fieldName(ArrowType type) {
return Types.getMinorTypeForArrowType(type).name().toLowerCase();
}

private FieldType fieldType(MinorType type) {
return FieldType.nullable(type.getType());
}

private <T extends FieldVector> T addOrGet(MinorType minorType, Class<T> c) {
return internalMap.addOrGet(fieldName(minorType), fieldType(minorType), c);
private <T extends FieldVector> T addOrGet(ArrowType type, Class<T> c) {
return internalMap.addOrGet(fieldName(type), FieldType.nullable(type), c);
}

@Override
Expand Down Expand Up @@ -177,7 +173,7 @@ public long getOffsetBufferAddress() {
public MapVector getMap() {
if (mapVector == null) {
int vectorCount = internalMap.size();
mapVector = addOrGet(MinorType.MAP, MapVector.class);
mapVector = addOrGet(MinorType.MAP.getType(), MapVector.class);
if (internalMap.size() > vectorCount) {
mapVector.allocateNew();
if (callBack != null) {
Expand All @@ -200,7 +196,31 @@ public MapVector getMap() {
public ${name}Vector get${name}Vector() {
if (${uncappedName}Vector == null) {
int vectorCount = internalMap.size();
${uncappedName}Vector = addOrGet(MinorType.${name?upper_case}, ${name}Vector.class);
${uncappedName}Vector = addOrGet(MinorType.${name?upper_case}.getType(), ${name}Vector.class);
if (internalMap.size() > vectorCount) {
${uncappedName}Vector.allocateNew();
if (callBack != null) {
callBack.doWork();
}
}
}
return ${uncappedName}Vector;
}
<#else>

private ${name}Vector ${uncappedName}Vector;

public ${name}Vector get${name}Vector() {
assert ${uncappedName}Vector != null;
return ${uncappedName}Vector;
}

public ${name}Vector get${name}Vector(<#list minor.typeParams as typeParam>${typeParam.type} ${typeParam.name}<#sep>, </#list>) {
ArrowType type = new ArrowType.${name}(<#list minor.typeParams as typeParam>${typeParam.name}<#sep>, </#list>);

if (${uncappedName}Vector == null) {
int vectorCount = internalMap.size();
${uncappedName}Vector = addOrGet(type, ${name}Vector.class);
if (internalMap.size() > vectorCount) {
${uncappedName}Vector.allocateNew();
if (callBack != null) {
Expand All @@ -210,14 +230,15 @@ public MapVector getMap() {
}
return ${uncappedName}Vector;
}

</#if>
</#list>
</#list>

public ListVector getList() {
if (listVector == null) {
int vectorCount = internalMap.size();
listVector = addOrGet(MinorType.LIST, ListVector.class);
listVector = addOrGet(MinorType.LIST.getType(), ListVector.class);
if (internalMap.size() > vectorCount) {
listVector.allocateNew();
if (callBack != null) {
Expand Down Expand Up @@ -499,10 +520,8 @@ public Object getObject(int index) {
<#assign name = minor.class?cap_first />
<#assign fields = minor.fields!type.fields />
<#assign uncappedName = name?uncap_first/>
<#if !minor.typeParams?? >
case ${name?upper_case}:
return get${name}Vector().getObject(index);
</#if>
</#list>
</#list>
case MAP:
Expand Down Expand Up @@ -599,13 +618,17 @@ public void setSafe(int index, UnionHolder holder) {
<#assign name = minor.class?cap_first />
<#assign fields = minor.fields!type.fields />
<#assign uncappedName = name?uncap_first/>
<#if !minor.typeParams?? >
<#if !minor.typeParams?? >
public void setSafe(int index, Nullable${name}Holder holder) {
setType(index, MinorType.${name?upper_case});
get${name}Vector().setSafe(index, holder);
}

</#if>
<#else>
public void setSafe(int index, Nullable${name}Holder holder) {
setType(index, MinorType.${name?upper_case});
get${name}Vector(<#list minor.typeParams as typeParam>holder.${typeParam.name}<#sep>, </#list>).setSafe(index, holder);
}
</#if>
</#list>
</#list>

Expand Down
Loading