[Kernel] [CC Refactor #1] Add ConfigurationProvider, TableIdentifier, TableBuilder APIs #3781

scottsand-db · 2024-10-18T20:15:30Z

NOTE: I won't be merging any of these PRs until the entire e2e implementation is complete and all PRs are approved

This is part of some stacked PRs:

[Kernel] [CC Refactor #1] Add ConfigurationProvider, TableIdentifier, TableBuilder APIs #3781
[Kernel] [CC Refactor #2] Add new Kernel CommitCoordinatorClient interface #3782

Which Delta project/connector is this regarding?

Description

New Interfaces/Classes

adds a new ConfigurationProvider interface, that will let kernel get per-session configurations in the future
adds a new TableIdentifier class, that kernel will pass on to Commit Coordinator services
adds a new TableBuilder interface, that that constructs kernel Table instances, with optional config and tableId params

Cleanup

Remove unecessary test references to code like TableImpl.forPath(engine, tablePath, () => System.currentTimeMillis) --> the System.currentTimeMilis is the default clock, anyways. No need to point to an internal API for that, too.

How was this patch tested?

Existing UTs.

Does this PR introduce any user-facing changes?

Yes. New ConfigurationProvider interface, new TableIdentifier class, new TableBuilder interface

scottsand-db · 2024-10-18T20:17:54Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/TableImpl.java

@@ -181,14 +197,6 @@ public CloseableIterator<ColumnarBatch> getChanges(
            });
  }

-  protected Path getDataPath() {


move protected methods to be below public methods

scottsand-db · 2024-10-18T20:41:58Z

kernel/kernel-api/src/main/java/io/delta/kernel/TableIdentifier.java

+
+package io.delta.kernel;
+
+/** Identifier for a table. e.g. <catalog> / <schema> / <tableName> */


@since 3.3.0

@Evolving

scottsand-db · 2024-10-18T21:34:06Z

kernel/kernel-api/src/main/java/io/delta/kernel/TableIdentifier.java

+
+package io.delta.kernel;
+
+/** Identifier for a table. e.g. <catalog> / <schema> / <tableName> */


[error] /home/runner/work/delta/delta/kernel/kernel-api/src/main/java/io/delta/kernel/TableIdentifier.java:19:1: error: unknown tag: catalog [error] /** Identifier for a table. e.g. <catalog> / <schema> / <tableName> */

raveeram-db

LGTM!

vkorukanti

There are some qns (not blocking)

vkorukanti · 2024-10-18T22:57:44Z

kernel/kernel-api/src/main/java/io/delta/kernel/TableIdentifier.java

+  /** Returns the name of the table. */
+  public String getName() {
+    return name;
+  }


toString (for printing into logs). equals, hashCode (may add if needed later, but I do see this is useful in near future).

vkorukanti · 2024-10-18T22:58:55Z

kernel/kernel-api/src/main/java/io/delta/kernel/config/ConfigurationProvider.java

+ * <p>These values are not Delta table properties. Delta table properties are stored in the Delta
+ * log metadata and apply globally to all readers and writers interacting with the table.


can you give an example of where this gets used or what sort of configuration can it contain?

First example usage

The implementation of

CommitCoordinatorClient Engine::getCommitCoordinatorClient( String commitCoordinatorName, ConfigurationProvider sessionConfig, Map<String, String> commitCoordinatorConf);

will use it to lookup which builder to use for the given commitCoordinatorName.

It will lookup a value like io.delta.kernel.commitCoordinatorBuilder.unity-catalog.impl

Second example usage

Then, the actual builder referenced above will lookup various keys / configurations needed to instantiate the client.

vkorukanti · 2024-10-18T23:01:01Z

kernel/kernel-api/src/main/java/io/delta/kernel/config/ConfigurationProvider.java

+   * @param key the configuration key
+   * @return {@code true} if the key exists, else {@code false}
+   */
+  boolean contains(String key);


contains is tricky to define for conf. There is always some default conf and what does it mean when call contains.

Is it better to just have Optional<String> get(String key) and avoid the multiple APIs.

That makes sense. Thanks!

vkorukanti · 2024-10-18T23:02:14Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/EmptyConfigurationProvider.java

+ * configuration is provided or expected.
+ */
+public class EmptyConfigurationProvider implements ConfigurationProvider {
+  @Override


can define just one static instance and use it everywhere rather than creating a new instance everytime.

that could be even in ConfigurationProvider (avoids a new file).

vkorukanti · 2024-10-18T23:03:00Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/TableBuilderImpl.java

+import java.util.Optional;
+
+public class TableBuilderImpl implements TableBuilder {
+  private final Engine engine;


we avoid this kind of pattern where Engine is stored in class member variables.

That makes sense. We will have to think about how to work around this, then

// Table.java static TableBuilder builder() { return new TableBuilderImpl(); } // TableBuilder.java public Table build(Engine engine, String path)

vkorukanti · 2024-10-18T23:03:53Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/TableBuilderImpl.java

+    requireNonNull(engine, "Expected non-null value for 'engine'");
+    requireNonNull(path, "Expected non-null value for 'path'");
+
+    this.engine = engine;


Suggested change

this.engine = engine;

this.engine = requireNonNull(engine, "blah blah");

vkorukanti · 2024-10-18T23:07:20Z

kernel/kernel-api/src/main/java/io/delta/kernel/config/ConfigurationProvider.java

+ * @since 3.3.0
+ */
+@Evolving
+public interface ConfigurationProvider {


what is the lifecycle of this object? Can Kernel maintain a reference to it? expected to release it after some event? can Engine update it while Kernel is using it (i.e return different values at different times for the same key)?

ebyhr · 2024-10-19T03:18:07Z

kernel/kernel-api/src/main/java/io/delta/kernel/TableIdentifier.java

+  /** The name of the table. */
+  private final String name;
+
+  /** The namespace of the table. e.g. <catalog> / <schema> */
+  private final String[] namespace;


nit: The order of constructor arguments & getters is namespace → name. I would recommend using the same order in fields.

Thanks! Will do! I'm still working on some more PRs to fully flesh out this design. Will get to this later.

ebyhr · 2024-10-19T03:21:41Z

kernel/kernel-api/src/main/java/io/delta/kernel/TableIdentifier.java

+    this.namespace = namespace;
+    this.name = name;


requireNonNull seems missing. Also, it would be nice to ensure non-empty in my opinion.

ebyhr · 2024-10-19T03:26:46Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/TableBuilderImpl.java

+    requireNonNull(engine, "Expected non-null value for 'engine'");
+    requireNonNull(path, "Expected non-null value for 'path'");


The existing message pattern of requireNonNull is {variable} is null. I would recommend using the same pattern for consistency.

ebyhr · 2024-10-19T03:27:47Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/TableBuilderImpl.java

+  private Optional<ConfigurationProvider> configProviderOpt = Optional.empty();
+  private Optional<TableIdentifier> tableIdOpt = Optional.empty();
+
+  /** Note: This is not for a public API. It's for internal testing use only. */
+  private Optional<Clock> clockOpt = Optional.empty();


There's no need to add opt suffix in my opinion. It's obvious from the type.

I hear that. This is certainly one of my personal preferences. I like variables being fully self-describing. The value isn't a Clock, but distinctly a Clock Optional. With this current naming convention, if we did a .map, I could refer to the inner value as clock, but if I had the outer variable also named clock, this would get confusing.

scottsand-db · 2024-10-21T16:10:40Z

kernel/kernel-api/src/main/java/io/delta/kernel/config/ConfigurationProvider.java

+ * @since 3.3.0
+ */
+@Evolving
+public interface ConfigurationProvider {


UC Commit Coordinator Client needs to be able to iterate through all configs

scottsand-db added 9 commits October 18, 2024 10:12

Create ConfigurationProvider.java

1485372

Create EmptyConfigurationProvider.java

5c16b28

Create TableIdentifier.java

778705d

Add copyright headers to 3 files

87e199c

Create TableBuilder.java

8cc78f9

Create TableBuilderImpl.java

21735a1

Update Table.java and TableImpl.java to use new TableBuilder

9e4e53a

Remove internal usages of TableImpl.forPath, use TableImpl.builder

ef353e1

javadocs

5d1f382

scottsand-db added kernel kernel-api-change labels Oct 18, 2024

scottsand-db requested review from vkorukanti and allisonport-db October 18, 2024 20:15

scottsand-db self-assigned this Oct 18, 2024

scottsand-db changed the title ~~[Kernel] [Commit Coordinator Refactor #1] Add ConfigurationProvider, TableIdentifier, TableBuilder APIs~~ [Kernel] [CC Refactor #1] Add ConfigurationProvider, TableIdentifier, TableBuilder APIs Oct 18, 2024

scottsand-db commented Oct 18, 2024

View reviewed changes

scottsand-db mentioned this pull request Oct 18, 2024

[Kernel] [CC Refactor #2] Add new Kernel CommitCoordinatorClient interface #3782

Closed

5 tasks

scottsand-db requested review from prakharjain09, sumeet-db and dhruvarya-db October 18, 2024 21:32

scottsand-db commented Oct 18, 2024

View reviewed changes

raveeram-db approved these changes Oct 18, 2024

View reviewed changes

vkorukanti approved these changes Oct 18, 2024

View reviewed changes

ebyhr reviewed Oct 19, 2024

View reviewed changes

scottsand-db commented Oct 21, 2024

View reviewed changes

scottsand-db closed this Oct 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Kernel] [CC Refactor #1] Add ConfigurationProvider, TableIdentifier, TableBuilder APIs #3781

[Kernel] [CC Refactor #1] Add ConfigurationProvider, TableIdentifier, TableBuilder APIs #3781

scottsand-db commented Oct 18, 2024 •

edited

Loading

scottsand-db Oct 18, 2024

scottsand-db Oct 18, 2024

scottsand-db Oct 18, 2024

scottsand-db Oct 18, 2024

raveeram-db left a comment

vkorukanti left a comment

vkorukanti Oct 18, 2024

vkorukanti Oct 18, 2024

scottsand-db Oct 18, 2024

vkorukanti Oct 18, 2024

scottsand-db Oct 18, 2024

vkorukanti Oct 18, 2024

vkorukanti Oct 18, 2024

vkorukanti Oct 18, 2024

scottsand-db Oct 18, 2024 •

edited

Loading

vkorukanti Oct 18, 2024

vkorukanti Oct 18, 2024

ebyhr Oct 19, 2024

scottsand-db Oct 21, 2024

ebyhr Oct 19, 2024

ebyhr Oct 19, 2024

ebyhr Oct 19, 2024

scottsand-db Oct 21, 2024

scottsand-db Oct 21, 2024


		package io.delta.kernel;

		/** Identifier for a table. e.g. <catalog> / <schema> / <tableName> */

		* <p>These values are not Delta table properties. Delta table properties are stored in the Delta
		* log metadata and apply globally to all readers and writers interacting with the table.

	this.engine = engine;
	this.engine = requireNonNull(engine, "blah blah");

		requireNonNull(engine, "Expected non-null value for 'engine'");
		requireNonNull(path, "Expected non-null value for 'path'");

[Kernel] [CC Refactor #1] Add ConfigurationProvider, TableIdentifier, TableBuilder APIs #3781

[Kernel] [CC Refactor #1] Add ConfigurationProvider, TableIdentifier, TableBuilder APIs #3781

Conversation

scottsand-db commented Oct 18, 2024 • edited Loading

Which Delta project/connector is this regarding?

Description

New Interfaces/Classes

Cleanup

How was this patch tested?

Does this PR introduce any user-facing changes?

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

raveeram-db left a comment

Choose a reason for hiding this comment

vkorukanti 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

First example usage

Second example usage

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

scottsand-db Oct 18, 2024 • 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

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

scottsand-db commented Oct 18, 2024 •

edited

Loading

scottsand-db Oct 18, 2024 •

edited

Loading