Skip to content

API: Add default value core api and schema serialization #4525

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
rzhang10 wants to merge 10 commits into from
Closed

API: Add default value core api and schema serialization #4525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0ebebaf
Add default value to API module
rzhang10 Apr 7, 2022
1b418f9
Add default value validation unit tests
rzhang10 Apr 8, 2022
c6459c1
Add default value serialization logic
rzhang10 Apr 8, 2022
fab4dee
Reformat code style for TestTypeDefaultValuesValidation
rzhang10 Apr 8, 2022
8c4c10b
Refactor API/Core module to let json validation/parser logic sit in core
rzhang10 Apr 11, 2022
b575ece
Implement Json to/from java schema representation round-trip conversion
rzhang10 Apr 12, 2022
2430dae
Use new api in PruneColumns
rzhang10 Apr 13, 2022
4b0baad
Convert date/time/ts to java in-mem int/long/long representations
rzhang10 Apr 14, 2022
43b7f7d
Remove not needed jackson module
rzhang10 Apr 15, 2022
a134f2b
Fix checkstyle
rzhang10 Apr 15, 2022
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
12 changes: 12 additions & 0 deletions api/src/main/java/org/apache/iceberg/expressions/Literal.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,18 @@ static Literal<BigDecimal> of(BigDecimal value) {
return new Literals.DecimalLiteral(value);
}

static Literal<Integer> ofDateLiteral(int value) {
Copy link
Contributor

@rdblue rdblue May 3, 2022

Choose a reason for hiding this comment

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

I don't think you should need any new literal constructors. You can always convert using to and the type.

Copy link
Contributor Author

@rzhang10 rzhang10 May 7, 2022

Choose a reason for hiding this comment

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

See my below comment.

return new Literals.DateLiteral(value);
}

static Literal<Long> ofTimeLiteral(long value) {
return new Literals.TimeLiteral(value);
}

static Literal<Long> ofTimestampLiteral(long value) {
return new Literals.TimestampLiteral(value);
}

/**
* Returns the value wrapped by this literal.
*/
Expand Down
10 changes: 9 additions & 1 deletion api/src/main/java/org/apache/iceberg/expressions/Literals.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,6 @@ public boolean equals(Object other) {
public int hashCode() {
return Objects.hashCode(value);
}

}

private abstract static class ComparableLiteral<C extends Comparable<C>> extends BaseLiteral<C> {
Expand Down Expand Up @@ -399,6 +398,8 @@ static class DateLiteral extends ComparableLiteral<Integer> {
public <T> Literal<T> to(Type type) {
if (type.typeId() == Type.TypeID.DATE) {
return (Literal<T>) this;
} else if (type.typeId() == Type.TypeID.STRING) {
return (Literal<T>) new StringLiteral(LocalDate.ofEpochDay(value()).format(DateTimeFormatter.ISO_LOCAL_DATE));
}
return null;
}
Expand All @@ -419,6 +420,9 @@ static class TimeLiteral extends ComparableLiteral<Long> {
public <T> Literal<T> to(Type type) {
if (type.typeId() == Type.TypeID.TIME) {
return (Literal<T>) this;
} else if (type.typeId() == Type.TypeID.STRING) {
return (Literal<T>) new StringLiteral(LocalTime.ofNanoOfDay(value() * 1000)
.format(DateTimeFormatter.ISO_LOCAL_TIME));
Copy link
Contributor

@rdblue rdblue May 3, 2022

Choose a reason for hiding this comment

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

I don't think this conversion should be done using literals. Instead, use the methods in DateTimeUtil and add any new methods there. This shouldn't require changes to the public API in expressions.

Copy link
Contributor Author

@rzhang10 rzhang10 May 7, 2022

Choose a reason for hiding this comment

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

Yeah since previously I remember you pointed me to the Literals code so I thought I might be able to utilize the existing literals conversion logic there.
My changes basically add the conversion from the 3 time/date-related type to string literal. Since I noticed the existing string literal code has the conversion to those date/time types, so I thought it might be a plus to add my current changes that can make the conversions more symmetric and enable people to do round-trip conversions?

WDYT?

Copy link
Contributor

@rdblue rdblue May 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this conversion should be done using literals. You can see how we're doing it here, but there is no need to change the Literals API. And there is certainly no need to add extra conversions to Literals. These are purposely limited.

}
return null;
}
Expand All @@ -443,6 +447,10 @@ public <T> Literal<T> to(Type type) {
case DATE:
return (Literal<T>) new DateLiteral((int) ChronoUnit.DAYS.between(
EPOCH_DAY, EPOCH.plus(value(), ChronoUnit.MICROS).toLocalDate()));
case STRING:
// Always return the literal without timezone.
return (Literal<T>) new StringLiteral(LocalDateTime.ofEpochSecond(value() / 1000000,
(int) (value() % 1000000) * 1000, ZoneOffset.UTC).format(DateTimeFormatter.ISO_LOCAL_DATE_TIME));
default:
}
return null;
Expand Down
6 changes: 4 additions & 2 deletions api/src/main/java/org/apache/iceberg/types/PruneColumns.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,12 @@ public Type struct(Types.StructType struct, List<Type> fieldResults) {
sameTypes = false; // signal that some types were altered
if (field.isOptional()) {
selectedFields.add(
Types.NestedField.optional(field.fieldId(), field.name(), projectedType, field.doc()));
Types.NestedField.optional(field.fieldId(), field.name(), projectedType, field.doc(),
field.initialDefaultValue(), field.writeDefaultValue()));
} else {
selectedFields.add(
Types.NestedField.required(field.fieldId(), field.name(), projectedType, field.doc()));
Types.NestedField.required(field.fieldId(), field.name(), projectedType, field.doc(),
field.initialDefaultValue(), field.writeDefaultValue()));
}
}
}
Expand Down
51 changes: 42 additions & 9 deletions api/src/main/java/org/apache/iceberg/types/Types.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,43 +415,64 @@ public int hashCode() {

public static class NestedField implements Serializable {
public static NestedField optional(int id, String name, Type type) {
return new NestedField(true, id, name, type, null);
return new NestedField(true, id, name, type, null, null, null);
}

public static NestedField optional(int id, String name, Type type, String doc) {
return new NestedField(true, id, name, type, doc);
return new NestedField(true, id, name, type, doc, null, null);
}

public static NestedField optional(int id, String name, Type type, String doc,
Object initialDefault, Object writeDefault) {
return new NestedField(true, id, name, type, doc, initialDefault, writeDefault);
}

public static NestedField required(int id, String name, Type type) {
return new NestedField(false, id, name, type, null);
return new NestedField(false, id, name, type, null, null, null);
}

public static NestedField required(int id, String name, Type type, String doc) {
return new NestedField(false, id, name, type, doc);
return new NestedField(false, id, name, type, doc, null, null);
}

public static NestedField required(int id, String name, Type type, String doc,
Object initialDefault, Object writeDefault) {
return new NestedField(false, id, name, type, doc, initialDefault, writeDefault);
}

public static NestedField of(int id, boolean isOptional, String name, Type type) {
return new NestedField(isOptional, id, name, type, null);
return new NestedField(isOptional, id, name, type, null, null, null);
}

public static NestedField of(int id, boolean isOptional, String name, Type type, String doc) {
return new NestedField(isOptional, id, name, type, doc);
return new NestedField(isOptional, id, name, type, doc, null, null);
}

public static NestedField of(int id, boolean isOptional, String name, Type type, String doc,
Object initialDefault, Object writeDefault) {
return new NestedField(isOptional, id, name, type, doc, initialDefault, writeDefault);
}

private final boolean isOptional;
private final int id;
private final String name;
private final Type type;
private final String doc;
private final Object initialDefault;
private final Object writeDefault;


private NestedField(boolean isOptional, int id, String name, Type type, String doc) {
private NestedField(boolean isOptional, int id, String name, Type type, String doc,
Object initialDefault, Object writeDefault) {
Copy link
Contributor

@rdblue rdblue May 3, 2022

Choose a reason for hiding this comment

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

Style: In Iceberg always start all argument lines at the same point, either all indented after the method declaration (aligned with boolean isOptional, ...) or all starting at the same continuation indent level. This mixes the two.

Copy link
Contributor

@rdblue rdblue May 18, 2022

Choose a reason for hiding this comment

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

@rzhang10, please don't resolve threads that are not updated yet.

Preconditions.checkNotNull(name, "Name cannot be null");
Preconditions.checkNotNull(type, "Type cannot be null");
this.isOptional = isOptional;
this.id = id;
this.name = name;
this.type = type;
this.doc = doc;
this.initialDefault = initialDefault;
this.writeDefault = writeDefault;
}

public boolean isOptional() {
Expand All @@ -462,7 +483,7 @@ public NestedField asOptional() {
if (isOptional) {
return this;
}
return new NestedField(true, id, name, type, doc);
return new NestedField(true, id, name, type, doc, initialDefault, writeDefault);
}

public boolean isRequired() {
Expand All @@ -473,7 +494,11 @@ public NestedField asRequired() {
if (!isOptional) {
return this;
}
return new NestedField(false, id, name, type, doc);
return new NestedField(false, id, name, type, doc, initialDefault, writeDefault);
}

public NestedField updateWriteDefault(Object newWriteDefault) {
return new NestedField(isOptional, id, name, type, doc, initialDefault, newWriteDefault);
}

public int fieldId() {
Expand All @@ -492,6 +517,14 @@ public String doc() {
return doc;
}

public Object initialDefaultValue() {
Copy link
Contributor

@rdblue rdblue May 3, 2022

Choose a reason for hiding this comment

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

Do we need Value here? It seems redundant. Maybe just initialDefault and writeDefault.

return initialDefault;
}

public Object writeDefaultValue() {
return writeDefault;
}

@Override
public String toString() {
return String.format("%d: %s: %s %s",
Expand Down
Loading