-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Variants: Implement toString #12138
Variants: Implement toString #12138
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,4 +34,20 @@ public interface VariantMetadata extends Variants.Serialized { | |
|
||
/** Returns the size of the metadata dictionary. */ | ||
int dictionarySize(); | ||
|
||
static String asString(VariantMetadata metadata) { | ||
StringBuilder builder = new StringBuilder(); | ||
Comment on lines
+38
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe just add as regular method rather than static function. That also will be consistent with other conversion like as asStructType() in Type.java. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can't be added in the interface because it would be a default that conflicts with an existing method ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. We need to add as default. We are adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only reason this is added as |
||
|
||
builder.append("VariantMetadata(dict={"); | ||
for (int i = 0; i < metadata.dictionarySize(); i += 1) { | ||
if (i > 0) { | ||
builder.append(", "); | ||
} | ||
|
||
builder.append(i).append(" => ").append(metadata.get(i)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Why not just use ":" here as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1. Basically we are creating an ID => object key string mapping. Seems fine to use ":" to be consistent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not an object, so this isn't really a key. It's a mapping from ID. The object strings mimic JSON (though that is not a guarantee) but this can't because the mapping "keys" are integers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry. I mean the the strings in the dictionary are the keys from the variant object. |
||
} | ||
builder.append("})"); | ||
|
||
return builder.toString(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,4 +38,23 @@ default Variants.PhysicalType type() { | |
default VariantObject asObject() { | ||
return this; | ||
} | ||
|
||
static String asString(VariantObject object) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. Probably add as regular class method rather than static funciton. |
||
StringBuilder builder = new StringBuilder(); | ||
|
||
builder.append("VariantObject(fields={"); | ||
boolean first = true; | ||
for (String field : object.fieldNames()) { | ||
if (first) { | ||
first = false; | ||
} else { | ||
builder.append(", "); | ||
} | ||
|
||
builder.append(field).append(": ").append(object.get(field)); | ||
} | ||
builder.append("})"); | ||
|
||
return builder.toString(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,11 @@ | |
*/ | ||
package org.apache.iceberg.variants; | ||
|
||
import java.nio.ByteBuffer; | ||
import org.apache.iceberg.relocated.com.google.common.io.BaseEncoding; | ||
import org.apache.iceberg.util.ByteBuffers; | ||
import org.apache.iceberg.util.DateTimeUtil; | ||
|
||
/** A primitive variant value. */ | ||
public interface VariantPrimitive<T> extends VariantValue { | ||
T get(); | ||
|
@@ -26,4 +31,23 @@ public interface VariantPrimitive<T> extends VariantValue { | |
default VariantPrimitive<?> asPrimitive() { | ||
return this; | ||
} | ||
|
||
private String valueAsString() { | ||
switch (type()) { | ||
case DATE: | ||
return DateTimeUtil.daysToIsoDate((Integer) get()); | ||
case TIMESTAMPTZ: | ||
return DateTimeUtil.microsToIsoTimestamptz((Long) get()); | ||
case TIMESTAMPNTZ: | ||
return DateTimeUtil.microsToIsoTimestamp((Long) get()); | ||
case BINARY: | ||
return BaseEncoding.base16().encode(ByteBuffers.toByteArray((ByteBuffer) get())); | ||
default: | ||
return String.valueOf(get()); | ||
} | ||
} | ||
|
||
static String asString(VariantPrimitive<?> primitive) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. Probably add as regular class method rather than static funciton. |
||
return "Variant(type=" + primitive.type() + ", value=" + primitive.valueAsString() + ")"; | ||
} | ||
} |
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 we add basic test coverage for the changes?
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.
These are for convenience and are not part of a contract. I don't think it is worth adding tests.