Generate delete file statistics for DLO strategy #287

cbb330 · 2025-02-15T22:29:26Z

Summary

problem: delete files are accumulating across tables due to trino default write behavior for UPDATE/MERGE/DELET statements

solution:

(this PR) - record delete file count/size per table and persist to table ✅
let the DLO strategy execute for many days to accumulate data 🕐
analyze the optimization algorithm and update the model to discover and act on tables who would benefit most from compacting their delete files 🕐
(future PR) - codify the algorithm updates into DLO strategy 🕐

outcome: databases accumulating delete files will be discovered and compacted appropriately to prevent degraded query impact

✨ bonus content ✨ :

I noticed there is a gap in DLO testing because manual validation must be done to verify the final form and contents of the DLO table, e.g. #284

so:

will a functional test into IntegrationTest by moving the DLO output table logic into StrategiesDAOInternal with save/load methods, and validated DLO finishes and outputs a table with contents and shape as expected 🕐
will add an acceptance test in internal app to validate schema is evolved correctly on the actual tables 🕐

this will help automate the testing for future changes and prevent bad changes from occuring due to validation being manually done

Changes

Testing Done

Manually Tested on local docker setup. Please include commands ran, and their output.
Added new tests for the changes made.
Updated existing tests to reflect the changes made.
No tests added or updated. Please explain why. If unsure, please feel free to ask for help.
Some other form of testing like staging or soak time in production. Please explain.

Additional Information

Breaking Changes
Deprecations
Large PR broken into smaller PRs, and PR plan linked in the description.

teamurko

Thank you, Christian. Looks good! Left a few questions

teamurko · 2025-02-17T23:52:44Z

...ark/src/main/java/com/linkedin/openhouse/jobs/spark/DataLayoutStrategyGeneratorSparkApp.java

+                  strategy.getPosDeleteFileCount(),
+                  strategy.getEqDeleteFileCount(),
+                  strategy.getPosDeleteFileBytes(),
+                  strategy.getEqDeleteFileBytes()));


Should it be a nested struct?

I see that these values are related, but a con is that the a struct would make ser/de have more steps.

is there another pro?

it's just cleaner, we can put it into struct, and during analysis read in trino, which is struct.field_name, non-blocking though

...src/test/java/com/linkedin/openhouse/jobs/spark/DataLayoutStrategyGeneratorSparkAppTest.java

libs/datalayout/src/main/java/com/linkedin/openhouse/datalayout/datasource/FileStat.java

...ark/src/main/java/com/linkedin/openhouse/jobs/spark/DataLayoutStrategyGeneratorSparkApp.java

sumedhsakdeo · 2025-02-18T21:12:49Z

...ayout/src/main/java/com/linkedin/openhouse/datalayout/persistence/StrategiesDaoInternal.java

@@ -0,0 +1,131 @@
+package com.linkedin.openhouse.datalayout.persistence;


Can we do DAO refactor into a separate PR and not mix with the delete file stats PR?

In fact let's do the delete file stats PR first, and then do DAO refactor, will need to think more about it and will take longer for that to land.

ok, will take this out of this PR,

will need to think more about it and will take longer for that to land.

I'd like to follow up with the refactor shortly after this PR deploys and works. whats the concern? the refactor is intended to not change behavior

No major concern.

sumedhsakdeo

Reduce scope of this PR to not include DAO refactor.

sumedhsakdeo · 2025-02-19T18:13:09Z

...ark/src/main/java/com/linkedin/openhouse/jobs/spark/DataLayoutStrategyGeneratorSparkApp.java

        } else {
          rows.add(
              String.format(
-                  "('%s', current_timestamp(), %f, %f, %f)",
-                  fqtn, strategy.getCost(), strategy.getGain(), strategy.getEntropy()));
+                  "('%s', current_timestamp(), %f, %f, %f, %d, %d, %d, %d)",


%d for bytes? Do you see a risk of overflow?

good question, no it should not.

initial search say %d is sufficient for Long.MAX_VALUE

diving deeper is not a clear code/doc answer. so I tested it on a range of very large numbers:
https://replit.com/join/xeprpdjctd-chrisbush747

cool, make sure you test the sql execution as well.

sure thing!

this code path isn't tested in main

i added unittests, they were green, but it required a bit of refactor, which we chose to remove in this PR

I will follow up with the tests again shortly after this PR is deployed

in the follow up, i will also ensure the LONG.MAX_VALUE is passed from pojo, string format, sql, and read back correctly

sumedhsakdeo · 2025-02-19T23:06:15Z

libs/datalayout/src/main/java/com/linkedin/openhouse/datalayout/datasource/FileStat.java

  private String path;
-  private long size;
+  private long sizeInBytes;


Is this serialized as size into tblproperty?

no, this value is only used in intermediate processing before being computed for DataLayoutStrategy (DLS) object. the DLS object is then serialized into tblproperty

here's the intermediate processing: https://github.com/linkedin/openhouse/pull/287/files#diff-454fe015ffc81fffb4f558b25e865396a81ebca0f3b2444e422d3379cf1ccf29R149

and here is where DLS is serialized into tblproperty:

openhouse/libs/datalayout/src/main/java/com/linkedin/openhouse/datalayout/persistence/StrategiesDaoTableProps.java

Line 32 in 5a0ec3c

fqtn, DATA_LAYOUT_STRATEGIES_PROPERTY_KEY, propValue));

teamurko

Thank you, looks clean, just one more question

teamurko · 2025-02-21T01:57:26Z

...ark/src/main/java/com/linkedin/openhouse/jobs/spark/DataLayoutStrategyGeneratorSparkApp.java

+                  strategy.getPosDeleteFileCount(),
+                  strategy.getEqDeleteFileCount(),
+                  strategy.getPosDeleteFileBytes(),
+                  strategy.getEqDeleteFileBytes()));


it's just cleaner, we can put it into struct, and during analysis read in trino, which is struct.field_name, non-blocking though

teamurko · 2025-02-21T02:07:36Z

libs/datalayout/src/main/java/com/linkedin/openhouse/datalayout/datasource/TableFileStats.java

          .partitionValues(partitionValues)
          .build();
    }
+
+    public static FileContent fromId(int id) {


Suggested change

public static FileContent fromId(int id) {

private static FileContent fromId(int id) {

teamurko · 2025-02-21T02:08:28Z

libs/datalayout/src/test/java/com/linkedin/openhouse/datalayout/e2e/IntegrationTest.java

+      List<DataLayoutStrategy> strategies = strategyGenerator.generatePartitionLevelStrategies();
+      Assertions.assertEquals(10, strategies.size());
+


why is this change?

doing a bit of side cleaning, since this is a bug

the test is for a partitioned table

the strategy generated at the end is a non-partitioned table

so i'm fixing it to generate a strategy for partitioned table

cbb330 force-pushed the main branch 3 times, most recently from 1898935 to 5867db2 Compare February 15, 2025 22:51

teamurko requested changes Feb 17, 2025

View reviewed changes

cbb330 force-pushed the main branch from 5867db2 to 70e2db3 Compare February 18, 2025 00:54

cbb330 commented Feb 18, 2025

View reviewed changes

...ark/src/main/java/com/linkedin/openhouse/jobs/spark/DataLayoutStrategyGeneratorSparkApp.java Outdated Show resolved Hide resolved

cbb330 force-pushed the main branch from 70e2db3 to 44d1f7b Compare February 18, 2025 01:32

dlo with delete file raw stats

1739504

cbb330 force-pushed the main branch from 44d1f7b to 1739504 Compare February 18, 2025 01:42

sumedhsakdeo reviewed Feb 18, 2025

View reviewed changes

sumedhsakdeo requested changes Feb 18, 2025

View reviewed changes

removing refactor / tests

5a0ec3c

sumedhsakdeo reviewed Feb 19, 2025

View reviewed changes

teamurko reviewed Feb 21, 2025

View reviewed changes

teamurko self-requested a review February 21, 2025 03:53

teamurko approved these changes Feb 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate delete file statistics for DLO strategy #287

Generate delete file statistics for DLO strategy #287

cbb330 commented Feb 15, 2025 •

edited

Loading

teamurko left a comment

teamurko Feb 17, 2025

cbb330 Feb 18, 2025

teamurko Feb 21, 2025

sumedhsakdeo Feb 18, 2025

sumedhsakdeo Feb 18, 2025

cbb330 Feb 18, 2025 •

edited

Loading

sumedhsakdeo Feb 18, 2025

sumedhsakdeo left a comment

sumedhsakdeo Feb 19, 2025

cbb330 Feb 19, 2025 •

edited

Loading

sumedhsakdeo Feb 19, 2025

cbb330 Feb 20, 2025

sumedhsakdeo Feb 19, 2025

cbb330 Feb 20, 2025

teamurko left a comment

teamurko Feb 21, 2025

teamurko Feb 21, 2025

teamurko Feb 21, 2025

cbb330 Feb 21, 2025

		@@ -0,0 +1,131 @@
		package com.linkedin.openhouse.datalayout.persistence;

	public static FileContent fromId(int id) {
	private static FileContent fromId(int id) {

		List<DataLayoutStrategy> strategies = strategyGenerator.generatePartitionLevelStrategies();
		Assertions.assertEquals(10, strategies.size());

Generate delete file statistics for DLO strategy #287

Are you sure you want to change the base?

Generate delete file statistics for DLO strategy #287

Conversation

cbb330 commented Feb 15, 2025 • edited Loading

Summary

Changes

Testing Done

Additional Information

teamurko left a 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

cbb330 Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sumedhsakdeo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbb330 Feb 19, 2025 • edited Loading

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

teamurko left a 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

cbb330 commented Feb 15, 2025 •

edited

Loading

cbb330 Feb 18, 2025 •

edited

Loading

cbb330 Feb 19, 2025 •

edited

Loading