Refactor Postgres Field Transformers and Collections #236

suddendust · 2025-09-24T11:51:12Z

Overview

This pull request refactors the Postgres field transformers to improve modularity and maintainability. It also introduces a clearer distinction between two different kinds of Postgres collections (Flat vs Nested), each with its own field transformation strategies.

Key Changes

Refactored Transformer Logic

Currently, FieldToPGTransformer handles transformation both for flat and nested PG collections in a single block of code. This responsibility is now divided b/w two classes:

FlatPostgresFieldTransformer: Handles flat PG collections. Fields names are simply returned as-is.
NestedPostgresColTransformer: Handles the case in which we store the entire doc inside the document column. Field transformations look like document -> attributes ->....

Distinct Postgres Collection Types
In line with the similar reasoning as above, we now have two Postgres collections, FlatPostgresCollection and NestedPostgresCollection, each handling their own query methods.
Changes to PostgresQueryParser: PostgresQueryParser now accepts the transformer directly in its constructor. If no transformer is supplied, it falls back to the NestedPostgresColTransformer.

API Changes

We now have a new Datastore#getCollectionForType method that can be used to return the PG collection of a particular type.

Checklist

My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
Any dependent changes have been merged and published in downstream modules

This reverts commit 674d2ee.

This reverts commit f6cb67a.

This reverts commit 6e7795d.

This reverts commit 957a897.

… into pg_subdocument

…ctor

codecov · 2025-09-24T11:52:41Z

Codecov Report

❌ Patch coverage is 83.08824% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.69%. Comparing base (3cc020b) to head (4f5e40d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ore/documentstore/postgres/PostgresCollection.java	77.55%	8 Missing and 3 partials ⚠️
...trace/core/documentstore/mongo/MongoDatastore.java	0.00%	3 Missing ⚠️
...documentstore/postgres/FlatPostgresCollection.java	81.81%	2 Missing ⚠️
...core/documentstore/postgres/PostgresDatastore.java	60.00%	1 Missing and 1 partial ⚠️
...y/v1/transformer/NestedPostgresColTransformer.java	91.66%	1 Missing and 1 partial ⚠️
...a/org/hypertrace/core/documentstore/Datastore.java	0.00%	1 Missing ⚠️
...ter/PostgresNotContainsRelationalFilterParser.java	0.00%	0 Missing and 1 partial ⚠️
...er/filter/PostgresNotInRelationalFilterParser.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #236      +/-   ##
============================================
+ Coverage     79.35%   79.69%   +0.33%     
- Complexity     1078     1097      +19     
============================================
  Files           208      212       +4     
  Lines          5276     5328      +52     
  Branches        453      450       -3     
============================================
+ Hits           4187     4246      +59     
+ Misses          770      762       -8     
- Partials        319      320       +1

Flag	Coverage Δ
integration	`79.69% <83.08%> (+0.33%)`	⬆️
unit	`57.45% <59.55%> (+0.57%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

puneet-traceable · 2025-09-24T16:25:28Z

@suddendust Please ensure that the test coverage is there.

puneet-traceable · 2025-09-24T16:36:34Z

...documentstore/postgres/query/v1/parser/filter/PostgresNotContainsRelationalFilterParser.java

-        flatStructureCollection != null
-            && flatStructureCollection.equals(context.getTableIdentifier().getTableName());
-
+        context.getPgColTransformer() instanceof FlatPostgresFieldTransformer;


how is the context getting set?

In general, I'd prefer avoiding "instanceof" checks as much as possible (except the object is an instance of the Object class itself). Would it be possible to do it from the enum?
"instanceof" seems to violate the Liskov's Substitution Principle.

Alternatively, the same can be achieve using overriding if possible.

@puneet-traceable The context is set by the callers. For example, check PostgresFilterTypeExpressionVisitor#visit.

puneet-traceable · 2025-09-24T16:41:23Z

...documentstore/postgres/query/v1/parser/filter/PostgresRelationalFilterParserFactoryImpl.java

 import org.hypertrace.core.documentstore.postgres.query.v1.parser.filter.nonjson.field.PostgresInRelationalFilterParserNonJsonField;
+import org.hypertrace.core.documentstore.postgres.query.v1.transformer.FlatPostgresFieldTransformer;

 public class PostgresRelationalFilterParserFactoryImpl


Can we create a similar factory for Flat collection? and use that in flat collection flow?

puneet-traceable · 2025-09-24T16:43:05Z

...t-store/src/main/java/org/hypertrace/core/documentstore/postgres/FlatPostgresCollection.java

+  }
+
+  private PostgresQueryParser createParser(Query query) {
+    return new PostgresQueryParser(tableIdentifier, PostgresQueryExecutor.transformAndLog(query));


we need to create thos object every time?

Yes, parser maintains state on a per-query level. This is how it is today as well.

…lat and nested collections

puneet-traceable · 2025-09-26T12:19:50Z

document-store/src/main/java/org/hypertrace/core/documentstore/Datastore.java

+   *     compatibility)
+   * @return the corresponding collection impl
+   */
+  default Collection getCollectionForType(String collectionName, DocumentType documentType) {


can you please add comment on other methods as well.
for eg: what would be the behavior of getCollection(), createCollection()

I do not see a corresponding implementation change in MongoDatastore.
If we upgrade document-store in entity-service, is the expectation that we would getCollection for mongodb collection and getCollectionForType for postgres collection?

+1
How is this supposed to be used by the clients/consumers?

.is the expectation that we would getCollection for mongodb collection and getCollectionForType for postgres collection?

That's the expectation right now, yes. Now that I think of it, a better approach would be to perhaps, for Mongo, to return an instance of MongoCollection for type NESTED and an IllegalArgumentException otherwise.

@suresh-prakash This API is intended to be primarily used for SQL datastores like PG in which we can store both flat and nested documents. For Mongo, since we only support nested, it'll throw an IllegalArgumentException for FLAT doc types (as per this PR).

Maybe, I am a bit confused here still. The consumers are the ones who are choosing which of the Postgres FLAT/NESTED to use. So, why would the clients again ask the library which one is being used. I mean, the clients would already have this information with them, right?

suresh-prakash · 2025-09-29T11:00:37Z

...nt-store/src/integrationTest/java/org/hypertrace/core/documentstore/DocStoreQueryV1Test.java

+
+    // Verify total count matches expected (each document contributes its tag count)
+    int totalTags = tagCounts.values().stream().mapToInt(Integer::intValue).sum();
+    assertTrue(totalTags > 0, "Total tag count should be greater than 0");


Nit: Perhaps a personal coding style. Messages are generally helpful if it can't be easily inferred from the assertion failure. In all these places, I feel the test failure is good enough to capture the message.

@suresh-prakash I personally prefer having clear messages, but would rather stick to what norms we follow today. What do you suggest?

Well, not a big deal either ways. I just found it redundant since we can easily infer the failure from the line numbers.

suresh-prakash · 2025-09-29T11:01:46Z

document-store/src/main/java/org/hypertrace/core/documentstore/Datastore.java

+   *     compatibility)
+   * @return the corresponding collection impl
+   */
+  default Collection getCollectionForType(String collectionName, DocumentType documentType) {


+1
How is this supposed to be used by the clients/consumers?

suresh-prakash · 2025-09-29T11:03:00Z

...e/src/main/java/org/hypertrace/core/documentstore/postgres/query/v1/PostgresQueryParser.java

    if (fromClause.isPresent()) {
      startIndexOfSelection = fromClause.get().length();
      sqlBuilder.append(fromClause.get());
+      // Update transformer with unnest column mappings


Is this supposed to be a TODO comment?

This comment isn't relevant anymore, removed.

suresh-prakash · 2025-09-29T11:05:19Z

...documentstore/postgres/query/v1/parser/filter/PostgresNotContainsRelationalFilterParser.java

-        flatStructureCollection != null
-            && flatStructureCollection.equals(context.getTableIdentifier().getTableName());
-
+        context.getPgColTransformer() instanceof FlatPostgresFieldTransformer;


In general, I'd prefer avoiding "instanceof" checks as much as possible (except the object is an instance of the Object class itself). Would it be possible to do it from the enum?
"instanceof" seems to violate the Liskov's Substitution Principle.

suresh-prakash · 2025-09-29T11:05:31Z

.../core/documentstore/postgres/query/v1/parser/filter/PostgresNotInRelationalFilterParser.java

-    boolean isFirstClassField =
-        flatStructureCollection != null
-            && flatStructureCollection.equals(context.getTableIdentifier().getTableName());
+    boolean isFirstClassField = context.getPgColTransformer() instanceof FlatPostgresCollection;


Same everywhere.

Gotcha! Added a getDocumentType to PostgresPGTransformer to determine what kind of documents that transformer is handling (flat vs nested).

suresh-prakash · 2025-09-29T11:06:34Z

...documentstore/postgres/query/v1/parser/filter/PostgresNotContainsRelationalFilterParser.java

-        flatStructureCollection != null
-            && flatStructureCollection.equals(context.getTableIdentifier().getTableName());
-
+        context.getPgColTransformer() instanceof FlatPostgresFieldTransformer;


Alternatively, the same can be achieve using overriding if possible.

suresh-prakash · 2025-09-29T11:08:30Z

...nt-store/src/integrationTest/java/org/hypertrace/core/documentstore/DocStoreQueryV1Test.java

+
+  @ParameterizedTest
+  @ArgumentsSource(PostgresProvider.class)
+  void testFlatPostgresCollectionFindAll(String dataStoreName) throws IOException {


If all these tests are specific to Postgres, worth nesting it under a nested class with @Nested annotation. That way, all related tests would be grouped together.

Also, it would be better to move all the tests (classes and methods) to the beginning of the class (before all the private methods).

Thanks for the suggestion! I have moved all flat PG tests to the nested class FlatPostgresCollectionTest.

This reverts commit 7f76f74.

…refactor

This reverts commit 40946ad.

suddendust · 2025-10-02T19:09:43Z

@suresh-prakash @puneet-traceable Requesting a re-review, thanks!

suddendust and others added 27 commits September 22, 2025 15:04

WIP

674d2ee

Revert "WIP"

f6cb67a

This reverts commit 674d2ee.

Reapply "WIP"

6e7795d

This reverts commit f6cb67a.

Revert "Reapply "WIP""

957a897

This reverts commit 6e7795d.

Reapply "Reapply "WIP""

502c373

This reverts commit 957a897.

WIP

0544358

WIP

82dc37f

Add default impl for getDocumentType

c0ed72a

WIP

e9f25da

Merge branch 'pg_subdocument' of github.com:suddendust/document-store…

e6be8b9

… into pg_subdocument

Rollback unnecessary changes

ee7678f

Added default DocumentType in Document.java

63b0dec

Address comments

7f91cf0

Merge branch 'main' into pg_subdocument

7376b56

Address comments

ea2aefd

Merge remote-tracking branch 'myfork/pg_subdocument' into pg_subdocument

e3ccb5b

Address docs

3e3e6cf

Adds more test cases in JSONDocumentTest.java

e6a6f7e

Spotless

483eb16

Added more test cases

e5926ea

Spotless

b261463

Add test case for PostgresResultIteratorWithBasicTypes

7cf8e17

Merge branch 'main' into pg_subdocument

9c474db

WIP

cb2ed18

Merge branch 'pg_subdocument' into poc

06fdc8f

WIP

fbce388

Merge branch 'main' of github.com:hypertrace/document-store into refa…

50b09b7

…ctor

suddendust requested review from avinashkolluru, kotharironak and skjindal93 as code owners September 24, 2025 11:51

suddendust added 2 commits September 24, 2025 17:26

WIP

0f8df71

WIP

7f7defe

suddendust changed the title ~~[Draft] Refactor Postgres Field Transformers~~ Refactor Postgres Field Transformers and Collections Sep 24, 2025

puneet-traceable reviewed Sep 24, 2025

View reviewed changes

suddendust added 3 commits September 26, 2025 11:41

Added test cases

13e591e

Added 1 more integration test case to ensure result consistency b/w f…

e1074b9

…lat and nested collections

Added test cases for unnest

77e1473

puneet-traceable reviewed Sep 26, 2025

View reviewed changes

suresh-prakash reviewed Sep 29, 2025

View reviewed changes

suddendust and others added 12 commits October 2, 2025 15:46

Merge branch 'main' into refactor

70763bb

Address comments except integration test

7f76f74

Revert "Address comments except integration test"

40946ad

This reverts commit 7f76f74.

Merge branch 'refactor' of github.com:suddendust/document-store into …

41a405f

…refactor

Reapply "Address comments except integration test"

1962554

This reverts commit 40946ad.

WIP

1903a4a

Test cases working

c3d0a32

WIP

0fa2a62

WIP

b6a4cf6

Add more test cases

d2fef95

Add more test cases

de09a90

WIP

4f5e40d

suddendust requested review from puneet-traceable and suresh-prakash October 2, 2025 19:09

suresh-prakash approved these changes Oct 3, 2025

View reviewed changes

suresh-prakash merged commit f0e2bef into hypertrace:main Oct 6, 2025
6 checks passed

Refactor Postgres Field Transformers and Collections #236

Refactor Postgres Field Transformers and Collections #236

Uh oh!

Conversation

suddendust commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Key Changes

API Changes

Checklist

Uh oh!

codecov bot commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

puneet-traceable commented Sep 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suddendust Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suddendust commented Oct 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

suddendust commented Sep 24, 2025 •

edited

Loading

codecov bot commented Sep 24, 2025 •

edited

Loading

suddendust Oct 2, 2025 •

edited

Loading