-
Notifications
You must be signed in to change notification settings - Fork 183
BED-5845 - Rework DAWGS Value Mapping #1411
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
Conversation
7078c51
to
657448e
Compare
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.
Mostly nits, really appreciated having the context specific commits. Made it really easy to see what was changing without feeling overloaded with context, and gave me a good idea of your process too!
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.
Changes look good, there's still quite a few places with the renamed graph.Result that name the variable scanner, may be worth the cleanup for consistency, but that's all that I found
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.
Thanks for all the extra diligence, love this changeset
Description
Generic value mapping in DAWGS is poorly supported and was hacked together in order to support the first iteration of generic Cypher search for users.
Originally this logic was written for Neo4j with its caveats and quirks. Addition of the PostgreSQL driver introduced significant drift and technical debt in several of these paths.
The PostgreSQL driver for DAWGS currently targets upstream OID matching to utilize the object mapping cache found in the pgx library. This can be examined in the Scan(...) function of the PostgreSQL DAWGS driver type. The performance benefit of this optimization is not great enough to justify both the Neo4j and PostgreSQL driver deviating on mapping to DAWGS types. This deviation also made generic mapping of query results far more difficult.
The Result and Scanner interface have been refactored to remove the current ValueMapper idiom and value mapping between both the Neo4j and PostgreSQL driver is now unified.
Motivation and Context
Resolves BED-5845
Value mapping in DAWGS contains a complexity trap related to certain query return shapes. This is most prevalent when using the generic Cypher search functionality. This trap leads to CPU saturation.
How Has This Been Tested?
This refactor is covered by our existing tests.
Types of changes
Checklist: