Respect target-max-file-size in Iceberg #10957

homar · 2022-02-04T22:55:19Z

Description

General information

Related issues, pull requests, and links

Documentation

(x) 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

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fixes ({issue}`10786`)

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergPageSink.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergParquetFileWriter.java

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java

findepi · 2022-02-08T10:46:19Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/parquet/ParquetFileWriter.java

@@ -89,6 +89,11 @@ public long getWrittenBytes()
        return parquetWriter.getWrittenBytes();
    }

+    public long getBufferedBytes()


Why we want to use it for Iceberg, but not for Hive?

As far as I saw hive does it little differently but it also takes bufferedbytes into account, just not directly

i am looking at

trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HivePageSink.java

Line 369 in 9ab09e9

if (bucketFunction != null || isTransactional || writer.getWrittenBytes() <= targetMaxFileSize.orElse(Long.MAX_VALUE)) {

and indeed ORC includes buffered bytes, and Parquet does not (#10957 (comment))

Still, we don't need this to be different for Hive vs Iceberg, right?

Ok sorry for a wrong explanation, so Hive uses implementations of FileWriter. There are many of them and as far as I understand it is hard for some of them to measure even already written bytes. For example here

trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/RecordFileWriter.java

Lines 135 to 157 in 6588a9e

public long getWrittenBytes()

{

if (recordWriter instanceof ExtendedRecordWriter) {

return ((ExtendedRecordWriter) recordWriter).getWrittenBytes();

}

if (committed) {

if (finalWrittenBytes != -1) {

return finalWrittenBytes;

}

try {

finalWrittenBytes = path.getFileSystem(conf).getFileStatus(path).getLen();

return finalWrittenBytes;

}

catch (IOException e) {

throw new UncheckedIOException(e);

}

}

// there is no good way to get this when RecordWriter is not yet committed

return 0;

}

I had no idea how to implement getWrittenBytes and getBufferedByte for some FileWriter implementations so I only focused on those used by Iceberg.

findepi · 2022-02-08T10:46:46Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergOrcFileWriter.java

+    @Override
+    public long getBufferedBytes()
+    {
+        return 0; //buffered bytes are already counted in written bytes by orcFileWriter


Sounds like undesired inconsistency. Would it be possible to fix it instead?

I will try to that in a separate PR after this one, ok ?

This might be as simple as adding parquetWriter.getBufferedBytes() at

trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/parquet/ParquetFileWriter.java

Line 89 in 9ab09e9

return parquetWriter.getWrittenBytes();

as a followup we may want to document or rename the method.

i would prefer to fix the inconsistency rather than introduce a method, if the end result is that the method is going to be removed soonish

yes it may be that simple but it will change behaviour of getWrittenBytes and I am not sure what depends on that. For example SortingFileWritter depends on that.. I'd prefer to do it as a separate PR to be easily able to see what is broken(if anything) by this change.

@findepi Do you agree?

yes it may be that simple but it will change behaviour of getWrittenBytes and I am not sure what depends on that.

For Parquet -- yes
for ORC -- no

SortingFileWritter depends on that..

if this is a problem, we already have a problem (for ORC), right?

findepi · 2022-02-08T10:48:02Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSessionProperties.java

@@ -345,4 +354,9 @@ public static boolean isProjectionPushdownEnabled(ConnectorSession session)
    {
        return session.getProperty(PROJECTION_PUSHDOWN_ENABLED, Boolean.class);
    }
+
+    public static Optional<Long> getTargetMaxFileSize(ConnectorSession session)


i asked you to have Optional here, but actually ... -- #10978

findepi · 2022-02-08T10:49:07Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java

+                .getMaterializedRows()
+                // as target_max_file_size is set to quite low value it can happen that created files are bigger,
+                // so just to be safe we check if it is not much bigger
+                .forEach(row -> assertThat((Long) row.getField(0)).isLessThan(maxSize.toBytes() * 3));


Use "is between". Let's verify no empty files got created.

findinpath · 2022-02-08T14:13:28Z

Can you please change the description of the PR to the new Trino PR template?

homar · 2022-02-08T14:16:49Z

Can you please change the description of the PR to the new Trino PR template?

and where is that ?

findinpath · 2022-02-08T14:21:30Z

https://github.com/trinodb/trino/blob/master/.github/pull_request_template.md

The PR template

homar · 2022-02-09T11:56:46Z

@findepi I pushed new version without getBufferedBytes method. I hope it is fine now

findepi · 2022-02-09T14:41:53Z

ci / maven-checks (17) failed. investigate || push empty please

findepi · 2022-02-09T14:43:33Z

lib/trino-parquet/src/main/java/io/trino/parquet/writer/ParquetWriter.java

@@ -161,7 +161,7 @@ private void writeChunk(Page page)
            flush();
            initColumnWriters();
            rows = 0;
-            bufferedBytes = columnWriters.stream().mapToLong(ColumnWriter::getBufferedBytes).sum();
+            bufferedBytes = columnWriters.stream().mapToLong(ColumnWriter::getRetainedBytes).sum();


Why the change here?

To make it behave same was as OrcWriter, and it actually makes sense to use retainedBytes here

getBufferedBytes seems to be "how much data i have"
getRetainedBytes seems to be "how much memory i have allocated"

at least this is what i see in FixedLenByteArrayPlainValuesWriter that i picked as example impl
i think we should use getBufferedBytes
(does OrcWriter need a change?)

@skrzypo987 do you know better?

ok I can change it to getBufferedBytes, should I also change it for OrcWriter in this PR ?

getBufferedBytes is the size that will actually be wirtten.

ok I can change it to getBufferedBytes, should I also change it for OrcWriter in this PR ?

can be separate.

findepi · 2022-02-09T14:44:07Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSessionProperties.java

@@ -69,6 +70,7 @@
    private static final String DYNAMIC_FILTERING_WAIT_TIMEOUT = "dynamic_filtering_wait_timeout";
    private static final String STATISTICS_ENABLED = "statistics_enabled";
    private static final String PROJECTION_PUSHDOWN_ENABLED = "projection_pushdown_enabled";
+    private static final String TARGET_MAX_FILE_SIZE = "target_max_file_size";


add an empty line before the list

findepi · 2022-02-09T14:45:04Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java

+        assertUpdate(session, createTableSql, 100000);
+        assertThat(query(format("SELECT count(*) FROM %s", tableName))).matches("VALUES (BIGINT '100000')");
+        List<String> updatedFiles = getActiveFiles(tableName);
+        assertThat(updatedFiles.size()).isGreaterThan(3);


Can we expect even more? like 10?

Using default settings we should not

it's not under default setting. you force small file size, so how many files are we getting?

currently there are around 30, I can change this value to 10

thanks, let's have 10

findepi · 2022-02-09T14:45:19Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java

+                .build();
+
+        assertUpdate(session, createTableSql, 100000);
+        assertThat(query(format("SELECT count(*) FROM %s", tableName))).matches("VALUES (BIGINT '100000')");


( in VALUES are redundant

homar · 2022-02-10T09:34:46Z

failing suite is not related, looks like there was an issue with container

cla-bot bot added the cla-signed label Feb 4, 2022

Fix formatting of BaseIcebergConnectorTest

90ff565

homar force-pushed the homar/respect_table_max_file_size_in_iceberg branch from 9adc6b0 to 277818f Compare February 4, 2022 22:58

homar requested a review from findepi February 4, 2022 22:58

github-actions bot added the tests:hive label Feb 4, 2022

homar force-pushed the homar/respect_table_max_file_size_in_iceberg branch 4 times, most recently from 5f601a1 to 32244aa Compare February 6, 2022 15:09

findepi reviewed Feb 7, 2022

View reviewed changes

homar force-pushed the homar/respect_table_max_file_size_in_iceberg branch 2 times, most recently from 3ada9c2 to a47b881 Compare February 8, 2022 10:28

findepi reviewed Feb 8, 2022

View reviewed changes

homar force-pushed the homar/respect_table_max_file_size_in_iceberg branch from a47b881 to f663c7a Compare February 8, 2022 12:20

homar force-pushed the homar/respect_table_max_file_size_in_iceberg branch from f663c7a to 5063c0e Compare February 9, 2022 11:55

homar force-pushed the homar/respect_table_max_file_size_in_iceberg branch from 5063c0e to 5b100ef Compare February 9, 2022 12:01

findepi approved these changes Feb 9, 2022

View reviewed changes

homar added 2 commits February 9, 2022 17:14

Adjust ParquetFileWriter.getWritteBytes to match OrcFileWriter behaviour

ac0bca4

Respect target-max-file-size in Iceberg

7696587

homar force-pushed the homar/respect_table_max_file_size_in_iceberg branch from 5b100ef to 7696587 Compare February 9, 2022 16:15

findepi approved these changes Feb 9, 2022

View reviewed changes

findepi merged commit 5edca27 into trinodb:master Feb 10, 2022

findepi mentioned this pull request Feb 10, 2022

Release notes for 371 #10941

Closed

homar deleted the homar/respect_table_max_file_size_in_iceberg branch February 10, 2022 09:47

github-actions bot added this to the 371 milestone Feb 10, 2022

This was referenced Feb 10, 2022

Add Trino 371 release notes #10943

Merged

Document supported Hive properties for Iceberg connector #11036

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect target-max-file-size in Iceberg #10957

Respect target-max-file-size in Iceberg #10957

homar commented Feb 4, 2022 •

edited

Loading

findepi Feb 8, 2022

homar Feb 8, 2022

findepi Feb 8, 2022

homar Feb 8, 2022

findepi Feb 8, 2022

homar Feb 8, 2022

findepi Feb 8, 2022

homar Feb 8, 2022

findepi Feb 9, 2022

findepi Feb 8, 2022

findepi Feb 8, 2022

findinpath commented Feb 8, 2022

homar commented Feb 8, 2022

findinpath commented Feb 8, 2022

homar commented Feb 9, 2022

findepi commented Feb 9, 2022

findepi Feb 9, 2022

homar Feb 9, 2022

findepi Feb 9, 2022

homar Feb 9, 2022

skrzypo987 Feb 9, 2022

findepi Feb 9, 2022

findepi Feb 9, 2022

findepi Feb 9, 2022

homar Feb 9, 2022

findepi Feb 9, 2022

homar Feb 9, 2022

findepi Feb 9, 2022

findepi Feb 9, 2022

homar commented Feb 10, 2022

	public long getWrittenBytes()
	{
	if (recordWriter instanceof ExtendedRecordWriter) {
	return ((ExtendedRecordWriter) recordWriter).getWrittenBytes();
	}

	if (committed) {
	if (finalWrittenBytes != -1) {
	return finalWrittenBytes;
	}

	try {
	finalWrittenBytes = path.getFileSystem(conf).getFileStatus(path).getLen();
	return finalWrittenBytes;
	}
	catch (IOException e) {
	throw new UncheckedIOException(e);
	}
	}

	// there is no good way to get this when RecordWriter is not yet committed
	return 0;
	}

Respect target-max-file-size in Iceberg #10957

Respect target-max-file-size in Iceberg #10957

Conversation

homar commented Feb 4, 2022 • edited Loading

Description

General information

Related issues, pull requests, and links

Documentation

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findinpath commented Feb 8, 2022

homar commented Feb 8, 2022

findinpath commented Feb 8, 2022

homar commented Feb 9, 2022

findepi commented Feb 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homar commented Feb 10, 2022

homar commented Feb 4, 2022 •

edited

Loading