Skip to content

Delta cleanup#21036

Merged
findepi merged 11 commits into
trinodb:masterfrom
findepi:findepi/delta-bar
Mar 13, 2024
Merged

Delta cleanup#21036
findepi merged 11 commits into
trinodb:masterfrom
findepi:findepi/delta-bar

Conversation

@findepi
Copy link
Copy Markdown
Member

@findepi findepi commented Mar 12, 2024

No description provided.

@findepi findepi added the no-release-notes This pull request does not require release notes entry label Mar 12, 2024
@findepi findepi requested review from ebyhr and findinpath March 12, 2024 16:04
@cla-bot cla-bot Bot added the cla-signed label Mar 12, 2024
@github-actions github-actions Bot added tests:hive iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector bigquery BigQuery connector mongodb MongoDB connector labels Mar 12, 2024
@findepi findepi force-pushed the findepi/delta-bar branch 3 times, most recently from 4c89d54 to 2635964 Compare March 12, 2024 16:33
@github-actions github-actions Bot added the jdbc Relates to Trino JDBC driver label Mar 12, 2024
@findepi findepi force-pushed the findepi/delta-bar branch 4 times, most recently from b0a5391 to 6de49e0 Compare March 12, 2024 19:52
@findinpath
Copy link
Copy Markdown
Contributor

findinpath commented Mar 12, 2024

Pls remove the (now) rather misleading comment

trino/plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/transactionlog/TransactionLogAccess.java

Lines 230 to 235 in a69e3b4

// Deprecated in favor of the namesake method which allows checkpoint filtering
// to be able to perform partition pruning and stats projection on the `add` entries
// from the checkpoint.
/**
* @see #getActiveFiles(ConnectorSession, TableSnapshot, MetadataEntry, ProtocolEntry, TupleDomain, Optional)
*/

@findepi
Copy link
Copy Markdown
Member Author

findepi commented Mar 12, 2024

Pls remove the (now) rather misleading comment

i'd like to leave this up to you. a javadoc or code comment explaining when which one should be used, would be a good addition
@findepi findepi force-pushed the findepi/delta-bar branch from 6de49e0 to ded0d57 Compare March 12, 2024 21:12
ebyhr
ebyhr approved these changes Mar 12, 2024
View reviewed changes
Comment thread plugin/trino-accumulo/src/main/java/io/trino/plugin/accumulo/AccumuloMetadata.java

@Override
public Optional<ConnectorOutputMetadata> finishInsert(ConnectorSession session, ConnectorInsertTableHandle insertHandle, Collection<Slice> fragments, Collection<ComputedStatistics> computedStatistics)
public Optional<ConnectorOutputMetadata> finishInsert(
Copy link
Copy Markdown
Member

@ebyhr ebyhr Mar 12, 2024

Choose a reason for hiding this comment

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

Iceberg connector still uses the deprecated finishInsert method. Did you skip the change intentionally?
Copy link
Copy Markdown
Contributor

@findinpath findinpath Mar 13, 2024

Choose a reason for hiding this comment

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

nit: Blackhole is also not modified.
I'm guessing you are making these changes to eventually remove the deprecated method.
Copy link
Copy Markdown
Member Author

@findepi findepi Mar 13, 2024

Choose a reason for hiding this comment

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

Iceberg requires more changes, because finishInsert is used internally by create table flow
Copy link
Copy Markdown
Member Author

@findepi findepi Mar 13, 2024

Choose a reason for hiding this comment

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

and yes, there is still a bit more work tbd to remove the old overload
findinpath
findinpath approved these changes Mar 13, 2024
View reviewed changes
Comment thread plugin/trino-accumulo/src/main/java/io/trino/plugin/accumulo/AccumuloMetadata.java

@Override
public Optional<ConnectorOutputMetadata> finishInsert(ConnectorSession session, ConnectorInsertTableHandle insertHandle, Collection<Slice> fragments, Collection<ComputedStatistics> computedStatistics)
public Optional<ConnectorOutputMetadata> finishInsert(
Copy link
Copy Markdown
Contributor

@findinpath findinpath Mar 13, 2024

Choose a reason for hiding this comment

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

nit: Blackhole is also not modified.
I'm guessing you are making these changes to eventually remove the deprecated method.
@findepi findepi merged commit 0012e5a into trinodb:master Mar 13, 2024
@findepi findepi deleted the findepi/delta-bar branch March 13, 2024 08:13
@github-actions github-actions Bot added this to the 441 milestone Mar 13, 2024
@colebow colebow mentioned this pull request Mar 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bigquery BigQuery connector cla-signed delta-lake Delta Lake connector hive Hive connector iceberg Iceberg connector jdbc Relates to Trino JDBC driver mongodb MongoDB connector no-release-notes This pull request does not require release notes entry

3 participants

@findepi @findinpath @ebyhr