Skip to content
This repository was archived by the owner on Jun 15, 2021. It is now read-only.

Conversation

@cmathiesen
Copy link

This PR is based on feedback from the IF PR.

  • Added a generic "MetadataReader" that can be used to create an Iterable of Records for any of the metadata table types (snapshots, history, partitions)
  • ^This uses the table schema to convert StaticRows from the metadata table into Record's that the InputFormat RecordReader can interpret correctly using the new ObjectInspectors
  • ^This means we can get rid of the SnapshotIterable class entirely
  • I've added the tests for the Snapshots table and the History table for now

(Also apologies about the formatting changes, I'll revert those)

rdsr and others added 30 commits March 15, 2020 17:27
# Conflicts:
#	mr/src/main/java/org/apache/iceberg/mr/IcebergInputFormat.java
#	mr/src/test/java/org/apache/iceberg/mr/TestIcebergInputFormat.java
…stream-wip-pr

Prepare for upstream WIP PR
@cmathiesen cmathiesen requested review from massdosage and teabot July 1, 2020 10:48
Copy link

@massdosage massdosage left a comment

Choose a reason for hiding this comment

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

I think this looks pretty good

import org.apache.iceberg.FileScanTask;
import org.apache.iceberg.Schema;
import org.apache.iceberg.StructLike;
import org.apache.iceberg.*;

Choose a reason for hiding this comment

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

You need to run the whole build as I'm sure the checkstyle plugin will moan about this.

default:
throw new UnsupportedOperationException(
String.format("Cannot read %s file: %s", file.format().name(), file.path()));
String.format("Cannot read %s file: %s", file.format().name(), file.path()));

Choose a reason for hiding this comment

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

The previous indentation here is more what Iceberg uses.

.filter(task.residual())
.caseSensitive(caseSensitive)
.split(task.start(), task.length());
.read(inputFile)

Choose a reason for hiding this comment

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

Ditto for all the indentation changes here and below.

List<Object[]> result = shell.executeStatement("SELECT * FROM source_db.table_a");

assertEquals(3, result.size());
}

Choose a reason for hiding this comment

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

Can we remove the @ignore annotations from the tests below?

.toString());

List<Object[]> result = shell.executeStatement("SELECT * FROM source_db.table_a__snapshots");
List<Object[]> result = shell.executeStatement("SELECT * FROM source_db.table_a");

Choose a reason for hiding this comment

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

Is there anything we can verify to check that it actually read a history table rather than just a normal table? Are there some fields that are unique to the history who's values we can check?


List<Object[]> result = shell.executeStatement("SELECT * FROM source_db.table_a");

assertEquals(3, result.size());

Choose a reason for hiding this comment

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

Is there anything we can verify to check that it actually read a snapshot table rather than just a normal table? Are there some fields that are unique to the snapshot who's values we can check?

@massdosage massdosage changed the base branch from if-all-the-things to system-tables November 19, 2020 15:52
Copy link

@massdosage massdosage left a comment

Choose a reason for hiding this comment

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

Merging this branch into an EG branch so it can be worked on from within this org

@massdosage massdosage merged commit 66b44f4 into ExpediaGroup:system-tables Nov 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants