Skip to content

Commit

Permalink
Remove partition filters in DLO lib/app (#171)
Browse files Browse the repository at this point in the history
## Summary
Generating DLO strategy is relatively cheap, and it's useful to have it
on non-partitioned tables as well in order to evaluate if it's worth
optimizing the table writer.

## Changes

- [ ] Client-facing API Changes
- [ ] Internal API Changes
- [x] Bug Fixes
- [ ] New Features
- [ ] Performance Improvements
- [ ] Code Style
- [ ] Refactoring
- [ ] Documentation
- [ ] Tests

For all the boxes checked, please include additional details of the
changes made in this pull request.

## Testing Done
<!--- Check any relevant boxes with "x" -->

- [ ] Manually Tested on local docker setup. Please include commands
ran, and their output.
- [ ] Added new tests for the changes made.
- [x] 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.

For all the boxes checked, include a detailed description of the testing
done for the changes made in this pull request.

# Additional Information

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

For all the boxes checked, include additional details of the changes
made in this pull request.
  • Loading branch information
teamurko authored Aug 21, 2024
1 parent d16c179 commit 8619c9f
Show file tree
Hide file tree
Showing 4 changed files with 7 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,7 @@ protected GetTableResponseBody getTable(TableMetadata tableMetadata) {
*/
public boolean canRunDataLayoutStrategyGeneration(TableMetadata tableMetadata) {
GetTableResponseBody response = getTable(tableMetadata);
return response != null
&& checkCreationTimeEligibility(response)
&& isPrimaryTable(response)
&& (response.getTimePartitioning() != null || response.getClustering() != null);
return response != null && checkCreationTimeEligibility(response) && isPrimaryTable(response);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ void testCanRunDataLayoutStrategyGeneration() {
Assertions.assertTrue(
client.canRunDataLayoutStrategyGeneration(
TableMetadata.builder().dbName(testDbName).tableName(testTableNameClustered).build()));
Assertions.assertFalse(
Assertions.assertTrue(
client.canRunDataLayoutStrategyGeneration(
TableMetadata.builder().dbName(testDbName).tableName(testTableName).build()));
Assertions.assertFalse(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,6 @@ public class OpenHouseDataLayoutStrategyGenerator implements DataLayoutStrategyG
*/
@Override
public List<DataLayoutStrategy> generate() {
// skip single partition and non-partitioned tables
if (tablePartitionStats.get().count() <= 1) {
return Collections.emptyList();
}
return Collections.singletonList(generateCompactionStrategy());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,11 @@ public void testCompactionStrategyGenerationNonPartitioned() throws Exception {
.tablePartitionStats(tablePartitionStats)
.build();
List<DataLayoutStrategy> strategies = strategyGenerator.generate();
Assertions.assertEquals(0, strategies.size());
Assertions.assertEquals(1, strategies.size());
StrategiesDao dao = StrategiesDaoTableProps.builder().spark(spark).build();
dao.save(testTable, strategies);
List<DataLayoutStrategy> retrievedStrategies = dao.load(testTable);
Assertions.assertEquals(strategies, retrievedStrategies);
}
}

Expand Down

0 comments on commit 8619c9f

Please sign in to comment.