feat: Delay reading from parquet file when creating table and column location #6590

malhotrashivam · 2025-01-23T21:34:58Z

https://deephaven.atlassian.net/browse/DH-18174

malhotrashivam · 2025-01-23T21:41:50Z

...table/src/main/java/io/deephaven/engine/table/impl/locations/impl/AbstractTableLocation.java

@@ -90,21 +98,25 @@ public LivenessReferent asLivenessReferent() {
    @Override
    @NotNull
    public final Object getStateLock() {
+        initialize();


I am not sure if these initialize calls should be put here or in ParquetTableLocation

Adding it here makes the entire delayed initialization logic more generic and extensive for other table locations too.

Putting it in ParquetTableLocation would require each public method of ParquetTableLocation to have an initialize call , which seems excessive. But then the initialize logic is ParquetTableLocation specific, so I do want to put it there and not touch this class.

Do we need to initialize on the TableLocationState methods?

Does it need to be invoked in any of the "final" methods that AbstractTableLocation defines?

I see it called in handleUpdate. Is it necessary there? If yes, then this sort of abstraction either needs to be in one of

AbstractTableLocation

new abstraction based off of TableLocation

in ParquetTableLocation which implements TableLocation (but not AbstractTableLocation)

These uncertainties make me want to push the initialization logic into ParquetTableLocation, which I'm assuming should be possible even if it extends AbstractTableLocation?

devinrsmith · 2025-01-24T17:52:36Z

...table/src/main/java/io/deephaven/engine/table/impl/locations/impl/AbstractTableLocation.java

@@ -90,21 +98,25 @@ public LivenessReferent asLivenessReferent() {
    @Override
    @NotNull
    public final Object getStateLock() {
+        initialize();


Do we need to initialize on the TableLocationState methods?

...table/src/main/java/io/deephaven/engine/table/impl/locations/impl/AbstractTableLocation.java

devinrsmith · 2025-01-24T17:56:16Z

...table/src/main/java/io/deephaven/engine/table/impl/locations/impl/AbstractTableLocation.java

@@ -90,21 +98,25 @@ public LivenessReferent asLivenessReferent() {
    @Override
    @NotNull
    public final Object getStateLock() {
+        initialize();


Does it need to be invoked in any of the "final" methods that AbstractTableLocation defines?

I see it called in handleUpdate. Is it necessary there? If yes, then this sort of abstraction either needs to be in one of

AbstractTableLocation

new abstraction based off of TableLocation

in ParquetTableLocation which implements TableLocation (but not AbstractTableLocation)

These uncertainties make me want to push the initialization logic into ParquetTableLocation, which I'm assuming should be possible even if it extends AbstractTableLocation?

malhotrashivam · 2025-01-27T18:06:46Z

...s/parquet/table/src/main/java/io/deephaven/parquet/table/location/ParquetColumnLocation.java

-    public ColumnChunkPageStore<ATTR>[] getPageStores(
+    private ColumnChunkPageStore<ATTR>[] getPageStores(


The class is package private and these methods are public and not used anywhere else in the repo, so made them private.

devinrsmith · 2025-01-28T21:51:22Z

...table/src/main/java/io/deephaven/engine/table/impl/locations/impl/AbstractTableLocation.java

@@ -94,12 +94,12 @@ public final Object getStateLock() {
    }

    @Override
-    public final RowSet getRowSet() {
+    public RowSet getRowSet() {


You are missing the need to override getLastModifiedTimeMillis in this regime.

That said, I think this is a case where we do want to provide a protected method for callers to override, but only for the TableLocationState methods. This allows us to leave these implementations final. Something like

// ------------------------------------------------------------------------------------------------------------------ // TableLocationState implementation // ------------------------------------------------------------------------------------------------------------------ protected void initializeState() { } @Override @NotNull public final Object getStateLock() { initializeState(); return state.getStateLock(); } @Override public final RowSet getRowSet() { initializeState(); return state.getRowSet(); } @Override public final long getSize() { initializeState(); return state.getSize(); } @Override public final long getLastModifiedTimeMillis() { initializeState(); return state.getLastModifiedTimeMillis(); }

And then it is just a shim into your normal initialize.

@Override protected void initializeState() { initialize(); }

devinrsmith · 2025-01-28T22:00:52Z

...s/parquet/table/src/main/java/io/deephaven/parquet/table/location/ParquetColumnLocation.java

+    private boolean isInitialized;
+    private volatile boolean isInitializedVolatile;


I don't know why you are using a pattern that involves non-vol + vol. I doubt there is a big performance consideration; and if there was, I would want to investigate further.

devinrsmith · 2025-01-28T22:09:12Z

...s/parquet/table/src/main/java/io/deephaven/parquet/table/location/ParquetColumnLocation.java

+        this.isInitializedVolatile = false;
+    }
+
+    private void initialize() {


I think we should be more explicit here since we have a two-stage initialization from what I can tell. I would call this initializeReaders, or something like that. And then, instead of calling it from a bunch of different places, we should only need to call it in the methods that actually wants to read columnChunkReaders - that is, exists and fetchValues.

devinrsmith · 2025-01-28T22:12:53Z

...s/parquet/table/src/main/java/io/deephaven/parquet/table/location/ParquetColumnLocation.java

+            if (isInitialized) {
+                return;
+            }
+            tl().initialize();


I'm not sure why it is our responsibility to call tl().initialize(); this should be handled internally by the implementation IMO.

I think this is a crutch; tl().getRowGroupReaders() should be calling initialize if it needs to access rowGroupIndices.

devinrsmith · 2025-01-28T22:14:40Z

...s/parquet/table/src/main/java/io/deephaven/parquet/table/location/ParquetColumnLocation.java

@@ -58,7 +60,13 @@ final class ParquetColumnLocation<ATTR extends Values> extends AbstractColumnLoc
    private static final int MAX_PAGE_CACHE_SIZE = Configuration.getInstance()
            .getIntegerForClassWithDefault(ParquetColumnLocation.class, "maxPageCacheSize", 8192);

+    private final ParquetTableLocation parquetTableLocation;


not sure it's worth re-storing just so you can get the specific type; casting in tl() seems good enough to me.

devinrsmith · 2025-01-28T22:31:28Z

...ns/parquet/table/src/main/java/io/deephaven/parquet/table/location/ParquetTableLocation.java

        if (tableInfo == null) {
            return null;
        }


Not your code... but can tableInfo == null ever be true in this case?

devinrsmith · 2025-01-28T22:36:14Z

...ns/parquet/table/src/main/java/io/deephaven/parquet/table/location/ParquetTableLocation.java

+    private ParquetFileReader parquetFileReader;
+    private int[] rowGroupIndices;

-    private final RowGroup[] rowGroups;
-    private final RegionedPageStore.Parameters regionParameters;
-    private final Map<String, String[]> parquetColumnNameToPath;
+    private RowGroup[] rowGroups;
+    private RegionedPageStore.Parameters regionParameters;
+    private Map<String, String[]> parquetColumnNameToPath;

-    private final TableInfo tableInfo;
-    private final Map<String, GroupingColumnInfo> groupingColumns;
-    private final List<DataIndexInfo> dataIndexes;
-    private final Map<String, ColumnTypeInfo> columnTypes;
-    private final List<SortColumn> sortingColumns;
+    private TableInfo tableInfo;
+    private Map<String, GroupingColumnInfo> groupingColumns;
+    private List<DataIndexInfo> dataIndexes;
+    private Map<String, ColumnTypeInfo> columnTypes;
+    private List<SortColumn> sortingColumns;

-    private final String version;
+    private String version;


I'm tracing down this code more thoroughly... I want to make sure we group all the members that need initialization together. It's a bit obvious in this case, but probably worthwhile to be explicit with something like:

private volatile boolean isInitialized; // Access to all the following variables must be guarded by initialize() private ParquetFileReader parquetFileReader; private int[] rowGroupIndices; ... private String version; // -----------------------------------------------------------------

devinrsmith · 2025-01-28T22:39:02Z

...ns/parquet/table/src/main/java/io/deephaven/parquet/table/location/ParquetTableLocation.java

-    private final RowGroup[] rowGroups;
-    private final RegionedPageStore.Parameters regionParameters;
-    private final Map<String, String[]> parquetColumnNameToPath;
+    private RowGroup[] rowGroups;


A deeper dive into this code has revealed some things that we don't really need to store IMO; for example, rowGroups is only accessed in computeIndex which is only called once during initialize.

Let's audit every single member variable to make sure:

It's properly guarded by an initialize()

It actually needs to exist as a member variable

devinrsmith · 2025-01-28T22:43:27Z

...ns/parquet/table/src/main/java/io/deephaven/parquet/table/location/ParquetTableLocation.java

+    public final BasicDataIndex getDataIndex(@NotNull final String... columns) {
+        initialize();
+        return super.getDataIndex(columns);
+    }


This seems like a poorly scoped initialize; it is not clear what member variables this is guarding. Assuming the member variables are guarded correctly, anything that super.getDataIndex calls should already be properly guarded.

devinrsmith · 2025-01-28T22:44:39Z

...table/src/main/java/io/deephaven/engine/table/impl/locations/impl/AbstractTableLocation.java

@@ -222,7 +222,7 @@ private BasicDataIndex getDataIndex() {

    @Override
    @Nullable
-    public final BasicDataIndex getDataIndex(@NotNull final String... columns) {
+    public BasicDataIndex getDataIndex(@NotNull final String... columns) {


As mentioned in the implementation, the implementation should not need to override this to properly initialize itself. We should leave this final.

malhotrashivam added 5 commits January 22, 2025 20:23

Lazy initialization added for ParquetTableLocation

e0f4e0d

Added lazy initialization for ParquetColumnLocation

f9b0916

Synchronized the initialization logic

24a544a

Updated some comments

dcb84ea

Added unit tests

6b4f803

malhotrashivam added feature request New feature or request parquet Related to the Parquet integration NoDocumentationNeeded ReleaseNotesNeeded Release notes are needed labels Jan 23, 2025

malhotrashivam added this to the 0.38.0 milestone Jan 23, 2025

malhotrashivam self-assigned this Jan 23, 2025

malhotrashivam requested review from devinrsmith, nbauernfeind, niloc132, rcaudy, cpwright, chipkent, jmao-denver and jcferretti as code owners January 23, 2025 21:35

malhotrashivam changed the base branch from main to release/v0.37.4 January 23, 2025 21:35

malhotrashivam changed the title ~~DH-18174: Delay reading from parquet file when creating table and column location~~ feat: Delay reading from parquet file when creating table and column location Jan 23, 2025

malhotrashivam marked this pull request as draft January 23, 2025 21:36

malhotrashivam commented Jan 23, 2025

View reviewed changes

Added a local variable along with the volatile

047896e

malhotrashivam changed the base branch from release/v0.37.4 to rc/v0.37.x January 24, 2025 17:45

devinrsmith reviewed Jan 24, 2025

View reviewed changes

Review with Devin

cc7ca09

malhotrashivam commented Jan 27, 2025

View reviewed changes

malhotrashivam added 2 commits January 27, 2025 19:12

Minor tweaks

1e0e36f

Removing redundant tests

b67b4f1

malhotrashivam marked this pull request as ready for review January 27, 2025 20:27

devinrsmith requested changes Jan 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Delay reading from parquet file when creating table and column location #6590

feat: Delay reading from parquet file when creating table and column location #6590

malhotrashivam commented Jan 23, 2025

malhotrashivam Jan 23, 2025 •

edited

Loading

devinrsmith Jan 24, 2025

devinrsmith Jan 24, 2025

devinrsmith Jan 24, 2025

devinrsmith Jan 24, 2025

malhotrashivam Jan 27, 2025

devinrsmith Jan 28, 2025

devinrsmith Jan 28, 2025

devinrsmith Jan 28, 2025

devinrsmith Jan 28, 2025

devinrsmith Jan 28, 2025

devinrsmith Jan 28, 2025

devinrsmith Jan 28, 2025

devinrsmith Jan 28, 2025

devinrsmith Jan 28, 2025

devinrsmith Jan 28, 2025

devinrsmith Jan 28, 2025

devinrsmith Jan 28, 2025

		public ColumnChunkPageStore<ATTR>[] getPageStores(
		private ColumnChunkPageStore<ATTR>[] getPageStores(

		private boolean isInitialized;
		private volatile boolean isInitializedVolatile;

feat: Delay reading from parquet file when creating table and column location #6590

Are you sure you want to change the base?

feat: Delay reading from parquet file when creating table and column location #6590

Conversation

malhotrashivam commented Jan 23, 2025

malhotrashivam Jan 23, 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

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

malhotrashivam Jan 23, 2025 •

edited

Loading