Skip to content
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

Avro: Add internal writer #11919

Merged
merged 4 commits into from
Jan 10, 2025
Merged
Changes from 1 commit
Commits
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
Next Next commit
Avro: Add internal writer
ajantha-bhat committed Jan 10, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
commit bb3a4c5677b7798ec8ba2957fd9d2cf1225801b0
117 changes: 117 additions & 0 deletions core/src/main/java/org/apache/iceberg/avro/BaseAvroSchemaVisitor.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.iceberg.avro;

import java.util.List;
import org.apache.avro.LogicalType;
import org.apache.avro.LogicalTypes;
import org.apache.avro.Schema;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;

abstract class BaseAvroSchemaVisitor extends AvroSchemaVisitor<ValueWriter<?>> {
Copy link
Contributor

@rdblue rdblue Jan 7, 2025

Choose a reason for hiding this comment

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

It's true that this is a visitor, but I think it's a best practice to use the class name to convey the purpose of this particular visitor.

In this case, it's building a writer tree so I think a name like BaseWriteBuilder would be more appropriate (based on what was copied here). You were right to use Base to indicate that it is an abstract base class that is shared across object models. I might revise that suggestion when I read through more to see if this is more specific, too.

Another reason is that the name BaseAvroSchemaVisitor is misleading. This class builds a writer tree and has a specific purpose. It wouldn't be used as a base for a visitor that has a different purpose, but the name implies that it could be.


protected abstract ValueWriter<?> createRecordWriter(List<ValueWriter<?>> fields);

protected abstract ValueWriter<?> fixedWriter(int size);

@Override
public ValueWriter<?> record(Schema record, List<String> names, List<ValueWriter<?>> fields) {
return createRecordWriter(fields);
}

@Override
public ValueWriter<?> union(Schema union, List<ValueWriter<?>> options) {
Preconditions.checkArgument(
options.contains(ValueWriters.nulls()),
"Cannot create writer for non-option union: %s",
union);
rdblue marked this conversation as resolved.
Show resolved Hide resolved
Preconditions.checkArgument(
options.size() == 2, "Cannot create writer for non-option union: %s", union);
if (union.getTypes().get(0).getType() == Schema.Type.NULL) {
return ValueWriters.option(0, options.get(1));
} else {
return ValueWriters.option(1, options.get(0));
}
}

@Override
public ValueWriter<?> array(Schema array, ValueWriter<?> elementWriter) {
if (array.getLogicalType() instanceof LogicalMap) {
ValueWriters.StructWriter<?> keyValueWriter = (ValueWriters.StructWriter<?>) elementWriter;
return ValueWriters.arrayMap(keyValueWriter.writer(0), keyValueWriter.writer(1));
}

return ValueWriters.array(elementWriter);
}

@Override
public ValueWriter<?> map(Schema map, ValueWriter<?> valueWriter) {
return ValueWriters.map(ValueWriters.strings(), valueWriter);
}

@Override
public ValueWriter<?> primitive(Schema primitive) {
LogicalType logicalType = primitive.getLogicalType();
if (logicalType != null) {
switch (logicalType.getName()) {
case "date":
return ValueWriters.ints();

case "time-micros":
return ValueWriters.longs();

case "timestamp-micros":
return ValueWriters.longs();

case "decimal":
LogicalTypes.Decimal decimal = (LogicalTypes.Decimal) logicalType;
return ValueWriters.decimal(decimal.getPrecision(), decimal.getScale());

case "uuid":
return ValueWriters.uuids();

default:
throw new IllegalArgumentException("Unsupported logical type: " + logicalType);
}
}

switch (primitive.getType()) {
case NULL:
return ValueWriters.nulls();
case BOOLEAN:
return ValueWriters.booleans();
case INT:
return ValueWriters.ints();
case LONG:
return ValueWriters.longs();
case FLOAT:
return ValueWriters.floats();
case DOUBLE:
return ValueWriters.doubles();
case STRING:
return ValueWriters.strings();
case FIXED:
return fixedWriter(primitive.getFixedSize());
case BYTES:
return ValueWriters.byteBuffers();
default:
throw new IllegalArgumentException("Unsupported type: " + primitive);
}
}
}
89 changes: 5 additions & 84 deletions core/src/main/java/org/apache/iceberg/avro/GenericAvroWriter.java
Original file line number Diff line number Diff line change
@@ -21,12 +21,9 @@
import java.io.IOException;
import java.util.List;
import java.util.stream.Stream;
import org.apache.avro.LogicalType;
import org.apache.avro.LogicalTypes;
import org.apache.avro.Schema;
import org.apache.avro.io.Encoder;
import org.apache.iceberg.FieldMetrics;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;

public class GenericAvroWriter<T> implements MetricsAwareDatumWriter<T> {
private ValueWriter<T> writer = null;
@@ -42,7 +39,7 @@ public static <D> GenericAvroWriter<D> create(Schema schema) {
@Override
@SuppressWarnings("unchecked")
public void setSchema(Schema schema) {
this.writer = (ValueWriter<T>) AvroSchemaVisitor.visit(schema, new WriteBuilder());
this.writer = (ValueWriter<T>) AvroSchemaVisitor.visit(schema, new GenericAvroSchemaVisitor());
}

@Override
@@ -55,92 +52,16 @@ public Stream<FieldMetrics> metrics() {
return writer.metrics();
}

private static class WriteBuilder extends AvroSchemaVisitor<ValueWriter<?>> {
private WriteBuilder() {}
private static class GenericAvroSchemaVisitor extends BaseAvroSchemaVisitor {

@Override
public ValueWriter<?> record(Schema record, List<String> names, List<ValueWriter<?>> fields) {
protected ValueWriter<?> createRecordWriter(List<ValueWriter<?>> fields) {
return ValueWriters.record(fields);
}

@Override
public ValueWriter<?> union(Schema union, List<ValueWriter<?>> options) {
Preconditions.checkArgument(
options.contains(ValueWriters.nulls()),
"Cannot create writer for non-option union: %s",
union);
Preconditions.checkArgument(
options.size() == 2, "Cannot create writer for non-option union: %s", union);
if (union.getTypes().get(0).getType() == Schema.Type.NULL) {
return ValueWriters.option(0, options.get(1));
} else {
return ValueWriters.option(1, options.get(0));
}
}

@Override
public ValueWriter<?> array(Schema array, ValueWriter<?> elementWriter) {
if (array.getLogicalType() instanceof LogicalMap) {
ValueWriters.StructWriter<?> keyValueWriter = (ValueWriters.StructWriter<?>) elementWriter;
return ValueWriters.arrayMap(keyValueWriter.writer(0), keyValueWriter.writer(1));
}

return ValueWriters.array(elementWriter);
}

@Override
public ValueWriter<?> map(Schema map, ValueWriter<?> valueWriter) {
return ValueWriters.map(ValueWriters.strings(), valueWriter);
}

@Override
public ValueWriter<?> primitive(Schema primitive) {
LogicalType logicalType = primitive.getLogicalType();
if (logicalType != null) {
switch (logicalType.getName()) {
case "date":
return ValueWriters.ints();

case "time-micros":
return ValueWriters.longs();

case "timestamp-micros":
return ValueWriters.longs();

case "decimal":
LogicalTypes.Decimal decimal = (LogicalTypes.Decimal) logicalType;
return ValueWriters.decimal(decimal.getPrecision(), decimal.getScale());

case "uuid":
return ValueWriters.uuids();

default:
throw new IllegalArgumentException("Unsupported logical type: " + logicalType);
}
}

switch (primitive.getType()) {
case NULL:
return ValueWriters.nulls();
case BOOLEAN:
return ValueWriters.booleans();
case INT:
return ValueWriters.ints();
case LONG:
return ValueWriters.longs();
case FLOAT:
return ValueWriters.floats();
case DOUBLE:
return ValueWriters.doubles();
case STRING:
return ValueWriters.strings();
case FIXED:
return ValueWriters.genericFixed(primitive.getFixedSize());
case BYTES:
return ValueWriters.byteBuffers();
default:
throw new IllegalArgumentException("Unsupported type: " + primitive);
}
protected ValueWriter<?> fixedWriter(int size) {
return ValueWriters.genericFixed(size);
}
}
}
Original file line number Diff line number Diff line change
@@ -205,7 +205,6 @@ public ValueReader<?> primitive(Pair<Integer, Type> partner, Schema primitive) {
case STRING:
return ValueReaders.strings();
case FIXED:
Copy link
Member Author

@ajantha-bhat ajantha-bhat Jan 6, 2025

Choose a reason for hiding this comment

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

As per Types.java, fixed also should be written and read as plain ByteBuffers.

Refer the testcase in TestInternalWriter

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this change.

Another check that this is correct is that ByteBuffer is used as the representation used by FixedLiteral, which must match the type of internal objects.

return ValueReaders.fixed(primitive);
case BYTES:
return ValueReaders.byteBuffers();
case ENUM:
74 changes: 74 additions & 0 deletions core/src/main/java/org/apache/iceberg/avro/InternalWriter.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.iceberg.avro;

import java.io.IOException;
import java.util.List;
import java.util.stream.Stream;
import org.apache.avro.Schema;
import org.apache.avro.io.Encoder;
import org.apache.iceberg.FieldMetrics;
import org.apache.iceberg.types.Type;

/**
* A Writer that consumes Iceberg's internal in-memory object model.
*
* <p>Iceberg's internal in-memory object model produces the types defined in {@link
* Type.TypeID#javaClass()}.
*/
public class InternalWriter<T> implements MetricsAwareDatumWriter<T> {
private ValueWriter<T> writer = null;

public static <D> InternalWriter<D> create(Schema schema) {
return new InternalWriter<>(schema);
}

InternalWriter(Schema schema) {
setSchema(schema);
}

@Override
@SuppressWarnings("unchecked")
public void setSchema(Schema schema) {
this.writer = (ValueWriter<T>) AvroSchemaVisitor.visit(schema, new GenericAvroSchemaVisitor());
}

@Override
public void write(T datum, Encoder out) throws IOException {
writer.write(datum, out);
}

@Override
public Stream<FieldMetrics> metrics() {
return writer.metrics();
}

private static class GenericAvroSchemaVisitor extends BaseAvroSchemaVisitor {
ajantha-bhat marked this conversation as resolved.
Show resolved Hide resolved

@Override
protected ValueWriter<?> createRecordWriter(List<ValueWriter<?>> fields) {
return ValueWriters.struct(fields);
}

@Override
protected ValueWriter<?> fixedWriter(int size) {
return ValueWriters.byteBuffers();
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't correct for the write path. In the read path, we want to broadly accept values and let validation happen later, so reading directly as ByteBuffer and not checking the fixed length is okay. However in the write path we do need to validate that the ByteBuffer being written is the expected fixed size, which ValueWriters.byteBuffers() (ByteBufferWriter) doesn't do. However, FixedWriter and GenericFixedWriter do check:

    @Override
    public void write(byte[] bytes, Encoder encoder) throws IOException {
      Preconditions.checkArgument(
          bytes.length == length,
          "Cannot write byte array of length %s as fixed[%s]",
          bytes.length,
          length);
      encoder.writeFixed(bytes);
    }

I think this needs a new ByteBufferWriter for fixed buffers:

  public static ValueWriter<ByteBuffer> fixedBuffers(int length) {
    return new FixedByteBufferWriter(length);
  }

  private static class FixedByteBufferWriter implements ValueWriter<ByteBuffer> {
    private final int length;

    private FixedByteBufferWriter(int length) {
      this.length = length;
    }

    @Override
    public void write(ByteBuffer bytes, Encoder encoder) throws IOException {
      Preconditions.checkArgument(
          bytes.remaining() == length,
          "Cannot write byte buffer of length %s as fixed[%s]",
          bytes.remaining(),
          length);
      encoder.writeBytes(bytes);
    }
  }

}
}
}
17 changes: 17 additions & 0 deletions core/src/main/java/org/apache/iceberg/avro/ValueWriters.java
Original file line number Diff line number Diff line change
@@ -32,6 +32,7 @@
import org.apache.avro.generic.IndexedRecord;
import org.apache.avro.io.Encoder;
import org.apache.avro.util.Utf8;
import org.apache.iceberg.StructLike;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.types.TypeUtil;
import org.apache.iceberg.util.DecimalUtil;
@@ -126,6 +127,10 @@ public static ValueWriter<IndexedRecord> record(List<ValueWriter<?>> writers) {
return new RecordWriter(writers);
}

public static ValueWriter<StructLike> struct(List<ValueWriter<?>> writers) {
return new StructLikeWriter(writers);
}

private static class NullWriter implements ValueWriter<Void> {
private static final NullWriter INSTANCE = new NullWriter();

@@ -484,4 +489,16 @@ protected Object get(IndexedRecord struct, int pos) {
return struct.get(pos);
}
}

private static class StructLikeWriter extends StructWriter<StructLike> {
@SuppressWarnings("unchecked")
ajantha-bhat marked this conversation as resolved.
Show resolved Hide resolved
private StructLikeWriter(List<ValueWriter<?>> writers) {
super(writers);
}

@Override
protected Object get(StructLike struct, int pos) {
return struct.get(pos, Object.class);
}
}
}
Loading