-
Notifications
You must be signed in to change notification settings - Fork 11
Refactor Postgres Field Transformers and Collections #236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
44 commits
Select commit
Hold shift + click to select a range
674d2ee
WIP
suddendust f6cb67a
Revert "WIP"
suddendust 6e7795d
Reapply "WIP"
suddendust 957a897
Revert "Reapply "WIP""
suddendust 502c373
Reapply "Reapply "WIP""
suddendust 0544358
WIP
suddendust 82dc37f
WIP
suddendust c0ed72a
Add default impl for `getDocumentType`
suddendust e9f25da
WIP
suddendust e6be8b9
Merge branch 'pg_subdocument' of github.com:suddendust/document-store…
suddendust ee7678f
Rollback unnecessary changes
suddendust 63b0dec
Added default DocumentType in Document.java
suddendust 7f91cf0
Address comments
suddendust 7376b56
Merge branch 'main' into pg_subdocument
suddendust ea2aefd
Address comments
suddendust e3ccb5b
Merge remote-tracking branch 'myfork/pg_subdocument' into pg_subdocument
suddendust 3e3e6cf
Address docs
suddendust e6a6f7e
Adds more test cases in JSONDocumentTest.java
suddendust 483eb16
Spotless
suddendust e5926ea
Added more test cases
suddendust b261463
Spotless
suddendust 7cf8e17
Add test case for PostgresResultIteratorWithBasicTypes
suddendust 9c474db
Merge branch 'main' into pg_subdocument
suddendust cb2ed18
WIP
suddendust 06fdc8f
Merge branch 'pg_subdocument' into poc
suddendust fbce388
WIP
suddendust 50b09b7
Merge branch 'main' of github.com:hypertrace/document-store into refa…
suddendust 0f8df71
WIP
suddendust 7f7defe
WIP
suddendust 13e591e
Added test cases
suddendust e1074b9
Added 1 more integration test case to ensure result consistency b/w f…
suddendust 77e1473
Added test cases for unnest
suddendust 70763bb
Merge branch 'main' into refactor
suddendust 7f76f74
Address comments except integration test
suddendust 40946ad
Revert "Address comments except integration test"
suddendust 41a405f
Merge branch 'refactor' of github.com:suddendust/document-store into …
suddendust 1962554
Reapply "Address comments except integration test"
suddendust 1903a4a
WIP
suddendust c3d0a32
Test cases working
suddendust 0fa2a62
WIP
suddendust b6a4cf6
WIP
suddendust d2fef95
Add more test cases
suddendust de09a90
Add more test cases
suddendust 4f5e40d
WIP
suddendust File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
470 changes: 470 additions & 0 deletions
470
...store/src/integrationTest/java/org/hypertrace/core/documentstore/DocStoreQueryV1Test.java
Large diffs are not rendered by default.
Oops, something went wrong.
12 changes: 12 additions & 0 deletions
12
document-store/src/integrationTest/resources/query/pg_flat_collection_insert.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| { | ||
| "statements": [ | ||
| "INSERT INTO \"myTestFlat\" (\n\"_id\", \"item\", \"price\", \"quantity\", \"date\", \"tags\", \"props\", \"sales\"\n) VALUES (\n1, 'Soap', 10, 2, '2014-03-01T08:00:00Z',\n'{\"hygiene\", \"personal-care\", \"premium\"}',\n'{\"colors\": [\"Blue\", \"Green\"], \"brand\": \"Dettol\", \"size\": \"M\", \"seller\": {\"name\": \"Metro Chemicals Pvt. Ltd.\", \"address\": {\"city\": \"Mumbai\", \"pincode\": 400004}}}',\nNULL\n)", | ||
| "INSERT INTO \"myTestFlat\" (\n\"_id\", \"item\", \"price\", \"quantity\", \"date\", \"tags\", \"props\", \"sales\"\n) VALUES (\n2, 'Mirror', 20, 1, '2014-03-01T09:00:00Z',\n'{\"home-decor\", \"reflective\", \"glass\"}',\nNULL,\nNULL\n)", | ||
| "INSERT INTO \"myTestFlat\" (\n\"_id\", \"item\", \"price\", \"quantity\", \"date\", \"tags\", \"props\", \"sales\"\n) VALUES (\n3, 'Shampoo', 5, 10, '2014-03-15T09:00:00Z',\n'{\"hair-care\", \"personal-care\", \"premium\", \"herbal\"}',\n'{\"colors\": [\"Black\"], \"brand\": \"Sunsilk\", \"size\": \"L\", \"seller\": {\"name\": \"Metro Chemicals Pvt. Ltd.\", \"address\": {\"city\": \"Mumbai\", \"pincode\": 400004}}}',\nNULL\n)", | ||
| "INSERT INTO \"myTestFlat\" (\n\"_id\", \"item\", \"price\", \"quantity\", \"date\", \"tags\", \"props\", \"sales\"\n) VALUES (\n4, 'Shampoo', 5, 20, '2014-04-04T11:21:39.736Z',\n'{\"hair-care\", \"budget\", \"bulk\"}',\nNULL,\nNULL\n)", | ||
| "INSERT INTO \"myTestFlat\" (\n\"_id\", \"item\", \"price\", \"quantity\", \"date\", \"tags\", \"props\", \"sales\"\n) VALUES (\n5, 'Soap', 20, 5, '2014-04-04T21:23:13.331Z',\n'{\"hygiene\", \"antibacterial\", \"family-pack\"}',\n'{\"colors\": [\"Orange\", \"Blue\"], \"brand\": \"Lifebuoy\", \"size\": \"S\", \"seller\": {\"name\": \"Hans and Co.\", \"address\": {\"city\": \"Kolkata\", \"pincode\": 700007}}}',\nNULL\n)", | ||
| "INSERT INTO \"myTestFlat\" (\n\"_id\", \"item\", \"price\", \"quantity\", \"date\", \"tags\", \"props\", \"sales\"\n) VALUES (\n6, 'Comb', 7.5, 5, '2015-06-04T05:08:13Z',\n'{\"grooming\", \"plastic\", \"essential\"}',\nNULL,\nNULL\n)", | ||
| "INSERT INTO \"myTestFlat\" (\n\"_id\", \"item\", \"price\", \"quantity\", \"date\", \"tags\", \"props\", \"sales\"\n) VALUES (\n7, 'Comb', 7.5, 10, '2015-09-10T08:43:00Z',\n'{\"grooming\", \"bulk\", \"wholesale\"}',\n'{\"colors\": [], \"seller\": {\"name\": \"Go Go Plastics\", \"address\": {\"city\": \"Kolkata\", \"pincode\": 700007}}}',\nNULL\n)", | ||
| "INSERT INTO \"myTestFlat\" (\n\"_id\", \"item\", \"price\", \"quantity\", \"date\", \"tags\", \"props\", \"sales\"\n) VALUES (\n8, 'Soap', 10, 5, '2016-02-06T20:20:13Z',\n'{\"hygiene\", \"budget\", \"basic\"}',\nNULL,\nNULL\n)" | ||
| ] | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
57 changes: 57 additions & 0 deletions
57
...tore/src/main/java/org/hypertrace/core/documentstore/postgres/FlatPostgresCollection.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| package org.hypertrace.core.documentstore.postgres; | ||
|
|
||
| import org.hypertrace.core.documentstore.CloseableIterator; | ||
| import org.hypertrace.core.documentstore.Document; | ||
| import org.hypertrace.core.documentstore.model.options.QueryOptions; | ||
| import org.hypertrace.core.documentstore.postgres.query.v1.PostgresQueryParser; | ||
| import org.hypertrace.core.documentstore.postgres.query.v1.transformer.FlatPostgresFieldTransformer; | ||
| import org.hypertrace.core.documentstore.query.Query; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** | ||
| * PostgreSQL collection implementation for flat documents. All fields are stored as top-level | ||
| * PostgreSQL columns. | ||
| */ | ||
| // todo: Throw unsupported op exception for all write methods | ||
| public class FlatPostgresCollection extends PostgresCollection { | ||
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(FlatPostgresCollection.class); | ||
|
|
||
| FlatPostgresCollection(final PostgresClient client, final String collectionName) { | ||
| super(client, collectionName); | ||
| } | ||
|
|
||
| @Override | ||
| public CloseableIterator<Document> query( | ||
| final org.hypertrace.core.documentstore.query.Query query, final QueryOptions queryOptions) { | ||
| PostgresQueryParser queryParser = createParser(query); | ||
| return queryWithParser(query, queryParser); | ||
| } | ||
|
|
||
| @Override | ||
| public CloseableIterator<Document> find( | ||
| final org.hypertrace.core.documentstore.query.Query query) { | ||
| PostgresQueryParser queryParser = createParser(query); | ||
| return queryWithParser(query, queryParser); | ||
| } | ||
|
|
||
| @Override | ||
| public long count( | ||
| org.hypertrace.core.documentstore.query.Query query, QueryOptions queryOptions) { | ||
| PostgresQueryParser queryParser = | ||
| new PostgresQueryParser( | ||
| tableIdentifier, | ||
| query, | ||
| new org.hypertrace.core.documentstore.postgres.query.v1.transformer | ||
| .FlatPostgresFieldTransformer()); | ||
| return countWithParser(query, queryParser); | ||
| } | ||
|
|
||
| private PostgresQueryParser createParser(Query query) { | ||
| return new PostgresQueryParser( | ||
| tableIdentifier, | ||
| PostgresQueryExecutor.transformAndLog(query), | ||
| new FlatPostgresFieldTransformer()); | ||
| } | ||
| } |
51 changes: 51 additions & 0 deletions
51
...re/src/main/java/org/hypertrace/core/documentstore/postgres/NestedPostgresCollection.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| package org.hypertrace.core.documentstore.postgres; | ||
|
|
||
| import org.hypertrace.core.documentstore.CloseableIterator; | ||
| import org.hypertrace.core.documentstore.Document; | ||
| import org.hypertrace.core.documentstore.model.options.QueryOptions; | ||
| import org.hypertrace.core.documentstore.postgres.query.v1.PostgresQueryParser; | ||
| import org.hypertrace.core.documentstore.postgres.query.v1.transformer.NestedPostgresColTransformer; | ||
| import org.hypertrace.core.documentstore.query.Query; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** | ||
| * PostgreSQL collection implementation for legacy document storage mode. Fields are stored within | ||
| * JSONB columns. | ||
| */ | ||
| public class NestedPostgresCollection extends PostgresCollection { | ||
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(NestedPostgresCollection.class); | ||
|
|
||
| NestedPostgresCollection(final PostgresClient client, final String collectionName) { | ||
| super(client, collectionName); | ||
| } | ||
|
|
||
| @Override | ||
| public CloseableIterator<Document> query( | ||
| final org.hypertrace.core.documentstore.query.Query query, final QueryOptions queryOptions) { | ||
| PostgresQueryParser queryParser = createParser(query); | ||
| return queryWithParser(query, queryParser); | ||
| } | ||
|
|
||
| @Override | ||
| public CloseableIterator<Document> find( | ||
| final org.hypertrace.core.documentstore.query.Query query) { | ||
| PostgresQueryParser queryParser = createParser(query); | ||
| return queryWithParser(query, queryParser); | ||
| } | ||
|
|
||
| @Override | ||
| public long count( | ||
| org.hypertrace.core.documentstore.query.Query query, QueryOptions queryOptions) { | ||
| PostgresQueryParser queryParser = createParser(query); | ||
| return countWithParser(query, queryParser); | ||
| } | ||
|
|
||
| private PostgresQueryParser createParser(Query query) { | ||
| return new PostgresQueryParser( | ||
| tableIdentifier, | ||
| PostgresQueryExecutor.transformAndLog(query), | ||
| new NestedPostgresColTransformer()); | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please add comment on other methods as well.
for eg: what would be the behavior of getCollection(), createCollection()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
How is this supposed to be used by the clients/consumers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
MongoCollectionfor typeNESTEDand anIllegalArgumentExceptionotherwise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
IllegalArgumentExceptionforFLATdoc types (as per this PR).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?