-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add all_files system table to the Iceberg connector #11206
The head ref may contain hidden characters: "add-all\u2014files"
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla. |
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 you add a short entry to https://github.com/trinodb/trino/blob/master/docs/src/main/sphinx/connector/iceberg.rst#metadata-tables as well?
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/AllFilesTable.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/AllFilesTable.java
Outdated
Show resolved
Hide resolved
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Manish Malhotra.
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Manish Malhotra.
|
@alexjo2144 thanks for the review! |
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Manish Malhotra.
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Manish Malhotra.
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla. |
closing to fix the git email issue |
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla. |
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla. |
@cla-bot check |
The cla-bot has been summoned, and re-checked this pull request! |
thanks a lot @martint! |
@alexjo2144 would you mind checking it, and seeing if changes are looking ok now? |
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.
Besides a few nitpicks, looks good to me.
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/AllFilesTable.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/AllFilesTable.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/AllFilesTable.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergSystemTables.java
Outdated
Show resolved
Hide resolved
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.
Only reviewed docs and left some suggestions on top of the good ideas from @findinpath
@findinpath @mosabua thanks for the review! will try to add the above changes today/tomorrow. |
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/AllFilesTable.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/FilesTable.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergSystemTables.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/AbstractFilesTable.java
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/AbstractFilesTable.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/AbstractFilesTable.java
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/AllFilesTable.java
Show resolved
Hide resolved
Build is red. Are the |
90d533d
to
db3a6e5
Compare
bed7c17
to
caf938e
Compare
caf938e
to
597faa6
Compare
Ok @ebyhr let me check your comments, thanks
…On Wed, Jul 20, 2022 at 10:31 PM Yuya Ebihara ***@***.***> wrote:
***@***.**** commented on this pull request.
I don't think we can merge this PR as-is. Please take a look at my comment
about $files table change.
------------------------------
In docs/src/main/sphinx/connector/iceberg.rst
<#11206 (comment)>:
> +
+``$all_files`` table
+^^^^^^^^^^^^^^^^^^^^
+
+The ``$all_files`` table exposes the valid data files of the Iceberg table.
+A valid data file is one that is readable from any snapshot currently tracked by the table.
+
+To retrieve the information about the data files from all the snapshots of the Iceberg table ``test_table`` use the following query::
+
+ SELECT * FROM "test_table$all_files"
+
+The output of the query has the columns, which is similar to the ``$files`` metadata table.
+
+.. code-block:: text
+
+ content | file_path | record_count | file_format | file_size_in_bytes | column_sizes | value_counts | null_value_counts | nan_value_counts | lower_bounds | upper_bounds | key_metadata | split_offsets | equality_ids
Please replace the order > record_count & file_format.
------------------------------
In docs/src/main/sphinx/connector/iceberg.rst
<#11206 (comment)>:
> + 0 | hdfs://hadoop-master:9000/user/hive/warehouse/test_table/data/c1=3/c2=2021-01-14/af9872b2-40f3-428f-9c87-186d2750d84e.parquet | 1 | PARQUET | 442 | {1=40, 2=40, 3=44} | {1=1, 2=1, 3=1} | {1=0, 2=0, 3=0} | <null> | {1=3, 2=2021-01-14, 3=1.3} | {1=3, 2=2021-01-14, 3=1.3} | <null> | <null> | <null>
+ 0 | hdfs://hadoop-master:9000/user/hive/warehouse/test_table/data/c1=3/c2=2021-01-14/af9872b2-40f3-428f-9c87-186d2750d84e.parquet | 1 | PARQUET | 442 | {1=40, 2=40, 3=44} | {1=1, 2=1, 3=1} | {1=0, 2=0, 3=0} | <null> | {1=3, 2=2021-01-14, 3=1.3} | {1=3, 2=2021-01-14, 3=1.2} | <null> | <null> | <null>
The 1st & 2nd file path are identical, but their upper bound differ. When
do we face such situation?
Providing those 2 files as the example might be little confusing and not
helpful. Removing the 2nd file row is fine in my opinion.
------------------------------
In
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/FilesTable.java
<#11206 (comment)>:
>
public class FilesTable
- implements SystemTable
+ extends AbstractFilesTable
Please separate a commit to introduce AbstractFilesTable.
------------------------------
In
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/FilesTable.java
<#11206 (comment)>:
>
- TableScan tableScan = icebergTable.newScan()
- .useSnapshot(snapshotId)
+ TableScan tableScan = filesTable
This change prevents accessing $files system table if delete-files
exists. Such failure didn't happen before this commit. You can reproduce by
below steps:
1. Start IcebergQueryRunner
2. Execute DELETE FROM region WHERE regionkey = 1
3. Execute SELECT * FROM "region$files"
class org.apache.iceberg.GenericDeleteFile cannot be cast to class org.apache.iceberg.DataFile (org.apache.iceberg.GenericDeleteFile and org.apache.iceberg.DataFile are in unnamed module of loader 'app')
Please add this test.
------------------------------
In
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergSystemTables.java
<#11206 (comment)>:
> + MaterializedResult expectedStatistics =
+ MaterializedResult.resultBuilder(getSession(), VARCHAR, BIGINT, new MapType(INTEGER, VARCHAR, new TypeOperators()),
+ new MapType(IntegerType.INTEGER, VARCHAR, new TypeOperators()))
+ .row("ORC", 1L, Map.of(1, "0", 2, "2019-09-08"), Map.of(1, "0", 2, "2019-09-08"))
+ .row("ORC", 2L, Map.of(1, "1", 2, "2019-09-09"), Map.of(1, "2", 2, "2019-09-09"))
+ .row("ORC", 1L, Map.of(1, "3", 2, "2019-09-09"), Map.of(1, "3", 2, "2019-09-09"))
+ .row("ORC", 2L, Map.of(1, "4", 2, "2019-09-10"), Map.of(1, "5", 2, "2019-09-10"))
+ .build();
+
+ assertEquals(result, expectedStatistics);
+ }
+
+ @test
+ public void testAllFilesTable()
+ {
+ try (TestTable table = new TestTable(getQueryRunner()::execute, "all_files_table", "(a int, b VARCHAR)", ImmutableList.of("(1, 'a')"))) {
int ... VARCHAR
nit: Let's uppercase int.
------------------------------
In
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergSystemTables.java
<#11206 (comment)>:
> + "VALUES ('content', 'integer', '', '')," +
+ "('file_path', 'varchar', '', '')," +
+ "('file_format', 'varchar', '', '')," +
+ "('record_count', 'bigint', '', '')," +
+ "('file_size_in_bytes', 'bigint', '', '')," +
+ "('column_sizes', 'map(integer, bigint)', '', '')," +
+ "('value_counts', 'map(integer, bigint)', '', '')," +
+ "('null_value_counts', 'map(integer, bigint)', '', '')," +
+ "('nan_value_counts', 'map(integer, bigint)', '', '')," +
+ "('lower_bounds', 'map(integer, varchar)', '', '')," +
+ "('upper_bounds', 'map(integer, varchar)', '', '')," +
+ "('key_metadata', 'varbinary', '', '')," +
+ "('split_offsets', 'array(bigint)', '', '')," +
+ "('equality_ids', 'array(integer)', '', '')");
+
+ assertQuerySucceeds("SELECT * FROM \"" + table.getName() + "$all_files\"");
Is it possible to verify all fields' values? Currently, this assertion
confirms only query success and some fields (e.g. key_metadata,
split_offsets & equality_ids) are null.
------------------------------
In
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergSystemTables.java
<#11206 (comment)>:
> + assertQuerySucceeds("SELECT * FROM \"" + table.getName() + "$all_files\"");
+
+ MaterializedResult filesAfterSingleRow = computeActual("SELECT file_format, record_count, lower_bounds, upper_bounds FROM \"" + table.getName() + "$files\"");
+ MaterializedResult allFilesAfterSingleRow = computeActual("SELECT file_format, record_count, lower_bounds, upper_bounds FROM \"" + table.getName() + "$all_files\"");
+ assertEquals(filesAfterSingleRow, allFilesAfterSingleRow);
+
+ MaterializedResult expectedFilesStatsAfterSingleRow =
+ MaterializedResult.resultBuilder(getSession(), VARCHAR, BIGINT, new MapType(INTEGER, VARCHAR, new TypeOperators()), new MapType(IntegerType.INTEGER, VARCHAR, new TypeOperators()))
+ .row("ORC", 1L, Map.of(1, "1", 2, "a"), Map.of(1, "1", 2, "a"))
+ .build();
+
+ assertEquals(filesAfterSingleRow, expectedFilesStatsAfterSingleRow);
+
+ Long snapshotAfterSingleRow = (Long) computeScalar("SELECT snapshot_id FROM \"" + table.getName() + "$snapshots\" ORDER BY committed_at DESC LIMIT 1");
+
+ assertUpdate("INSERT INTO " + table.getName() + " VALUES (2, 'b')", 1);
Please add DLETE statement to this test. It would be better to add other
statements and procedures too.
------------------------------
In
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergSystemTables.java
<#11206 (comment)>:
> + MaterializedResult filesAfterTwoRows = computeActual("SELECT file_format, record_count, lower_bounds, upper_bounds FROM \"" + table.getName() + "$files\" order by file_path");
+ MaterializedResult allFilesAfterTwoRows = computeActual("SELECT file_format, record_count, lower_bounds, upper_bounds FROM \"" + table.getName() + "$all_files\" order by file_path");
nit: Uppercase ORDER BY
------------------------------
In
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergSystemTables.java
<#11206 (comment)>:
> +
+ assertUpdate("INSERT INTO " + table.getName() + " VALUES (2, 'b')", 1);
+
+ MaterializedResult filesAfterTwoRows = computeActual("SELECT file_format, record_count, lower_bounds, upper_bounds FROM \"" + table.getName() + "$files\" order by file_path");
+ MaterializedResult allFilesAfterTwoRows = computeActual("SELECT file_format, record_count, lower_bounds, upper_bounds FROM \"" + table.getName() + "$all_files\" order by file_path");
+
+ MaterializedResult expectedAllFilesStatsAfterTwoRows =
+ MaterializedResult.resultBuilder(getSession(), VARCHAR, BIGINT, new MapType(INTEGER, VARCHAR, new TypeOperators()), new MapType(IntegerType.INTEGER, VARCHAR, new TypeOperators()))
+ .row("ORC", 1L, Map.of(1, "1", 2, "a"), Map.of(1, "1", 2, "a"))
+ .row("ORC", 1L, Map.of(1, "2", 2, "b"), Map.of(1, "2", 2, "b"))
+ .build();
+ assertEquals(filesAfterTwoRows, allFilesAfterTwoRows);
+
+ assertEquals(filesAfterTwoRows, expectedAllFilesStatsAfterTwoRows);
+
+ assertUpdate("CALL iceberg.system.rollback_to_snapshot('tpch', '" + table.getName() + "' , " + snapshotAfterSingleRow + ")");
nit: Remove a redundant space.
⬇️ Suggested change
- assertUpdate("CALL iceberg.system.rollback_to_snapshot('tpch', '" + table.getName() + "' , " + snapshotAfterSingleRow + ")");
+ assertUpdate("CALL iceberg.system.rollback_to_snapshot('tpch', '" + table.getName() + "', " + snapshotAfterSingleRow + ")");
------------------------------
In
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergSystemTables.java
<#11206 (comment)>:
> + MaterializedResult filesAfterRollback = computeActual("SELECT file_format, record_count, lower_bounds, upper_bounds FROM \"" + table.getName() + "$files\" order by file_path");
+ MaterializedResult allFilesAfterRollback = computeActual("SELECT file_format, record_count, lower_bounds, upper_bounds FROM \"" + table.getName() + "$all_files\" order by file_path");
nit: Uppercase ORDER BY
—
Reply to this email directly, view it on GitHub
<#11206 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AXQ2PYRPIJYTZYIMGIXQP63VVDOBXANCNFSM5PLUJCCA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
👋 @osscm - this PR has become inactive. If you're still interested in working on it, please let us know, and we can try to get reviewers to help with that. We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks. |
Thanks @bitsondatadev
Yes I think all_files will be useful for the community as well.
…On Fri, Nov 18, 2022 at 10:14 PM bitsondatadev ***@***.***> wrote:
👋 @osscm <https://github.com/osscm> - this PR has become inactive. If
you're still interested in working on it, please let us know, and we can
try to get reviewers to help with that.
We're working on closing out old and inactive PRs, so if you're too busy
or this has too many merge conflicts to be worth picking back up, we'll be
making another pass to close it out in a few weeks.
—
Reply to this email directly, view it on GitHub
<#11206 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AXQ2PYXIMC2QIXZIBJHJ7Y3WJBV4RANCNFSM5PLUJCCA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Great! We won't close it out then! I figured as such but we just need to gauge the interest in continuing all older PRs. I'll work on getting a maintainer to take a look! |
Thanks!
…On Sun, Nov 20, 2022 at 7:27 AM bitsondatadev ***@***.***> wrote:
Great! We won't close it out then! I figured as such but we just need to
gauge the interest in continuing all older PRs. I'll work on getting a
maintainer to take a look!
—
Reply to this email directly, view it on GitHub
<#11206 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AXQ2PYVULBAHFLYPCCB7SJ3WJI7PBANCNFSM5PLUJCCA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
- List of recommended split locations. | ||
* - ``equality_ids`` | ||
- ``array(integer)`` | ||
- The set of field IDs used for equality comparison in equality delete files |
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.
The above changes are unrelated to a new $all_files
system table. Please extract a commit.
|
||
SELECT * FROM "test_table$all_files" | ||
|
||
The output of the query has the columns, which is similar to the ``$files`` metadata table. |
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.
is similar to
I think they're the "same". Is my understanding wrong?
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.
yes @ebyhr its same.
are you suggesting to reword similar to
to same as
?
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.
I think so.
implements SystemTable | ||
{ | ||
private final ConnectorTableMetadata tableMetadata; | ||
private final Table icebergTable; |
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.
Change to protected
and use it from FilesTable
and AllFilesTable
import static org.apache.iceberg.MetadataTableType.ALL_DATA_FILES; | ||
import static org.apache.iceberg.MetadataTableUtils.createMetadataTableInstance; | ||
|
||
public class AllFilesTable |
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.
The class name is AllFilesTable
, but MetadataTableType
is ALL_DATA_FILES
. It would be nice to leave a comment.
Table allFilesTable = createMetadataTableInstance(getIcebergTable(), ALL_DATA_FILES); | ||
|
||
TableScan tableScan = allFilesTable | ||
.newScan() | ||
.includeColumnStats(); | ||
return tableScan; |
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.
Table allFilesTable = createMetadataTableInstance(getIcebergTable(), ALL_DATA_FILES); | |
TableScan tableScan = allFilesTable | |
.newScan() | |
.includeColumnStats(); | |
return tableScan; | |
return createMetadataTableInstance(getIcebergTable(), ALL_DATA_FILES).newScan() | |
.includeColumnStats(); |
|
||
assertEquals(result, expectedStatistics); | ||
} | ||
} |
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.
The above changes are unrelated to a new $all_files
system table. Please extract a commit.
} | ||
|
||
@Test | ||
public void testAllFilesTable() throws IOException |
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.
Move throws
to the next line.
MaterializedResult allFilesAfterSingleDelete = computeActual("SELECT content, file_format, file_size_in_bytes, record_count, column_sizes, value_counts," + | ||
"null_value_counts, nan_value_counts, key_metadata, split_offsets, equality_ids, lower_bounds, upper_bounds FROM \"" + table.getName() + "$all_files\" ORDER BY file_path"); |
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.
I would recommend using SELECT *
.
.row(0, "ORC", fileSizes[0], 1L, null, Map.of(1, Long.valueOf(1), 2, Long.valueOf(1)), Map.of(1, Long.valueOf(0), 2, Long.valueOf(0)), null, null, null, null, Map.of(1, "1", 2, "a"), Map.of(1, "1", 2, "a")) | ||
.row(0, "ORC", fileSizes[1], 1L, null, Map.of(1, Long.valueOf(1), 2, Long.valueOf(1)), Map.of(1, Long.valueOf(0), 2, Long.valueOf(0)), null, null, null, null, Map.of(1, "2", 2, "b"), Map.of(1, "2", 2, "b")) |
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.
Remove boxing.
} | ||
} | ||
|
||
private long[] getFileSizes(List<MaterializedRow> materializedRows) throws IOException |
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.
Move throws
to the next line.
Hey @osscm, are you still working on this? |
I'm going to close this out due to inactivity. Feel free to re-open if you want to pick this back up at any point in the future. |
Description
$all_files
system table support.$data_files
and$all_data_files
metadata tables. Trino is already supporting$files
.$all_files
table will include data_files from all the available snapshots to a table where the$files
metadata table includes only the current snapshot one.Use cases
This can be used when the user is debugging data issue. As in case when Trino/Spark is used for metadata/data optimization, then it can modify the metadata and data.
Another case is when snapshot is rolled back/future from Trino/Spark and now trying to understand what all data-files are present, and is there any implication because of the optimization/rollback operations.
This and
$all_manifests
can also be used to add the optimization features in Trino like purging orphan files or identifying partitions modified since last time, to implement moving window data-compaction feature. detailWhere we need to identify the
Design
Adding a new class
AllFilesTable
and used it as a parent forFilesTable
, as both will be implementing similar responsibility,.Testing
Added a test case in the existing
TestIcebergSystemTable
.Thought of using
rollback
to show case that$all_files
can give all the data_files, but then history was getting updated andtestHistoryTable
depends on the order and operations happen in thetestAllFilesTable
test. So have not used it. If we think, I can add it.syntax:
select * from "table$all_files"
Related issues, pull requests, and links
Documentation
( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.
Release notes
( ) No release notes entries required.
( ) Release notes entries required with the following suggested text: