patch - support for search queries on top of flattened postgres table…#223
Conversation
patelraj0602
requested review from
avinashkolluru,
kotharironak,
skjindal93 and
suresh-prakash
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #223 +/- ##
============================================
- Coverage 79.60% 78.55% -1.05%
- Complexity 1048 1069 +21
============================================
Files 204 206 +2
Lines 5084 5228 +144
Branches 416 449 +33
============================================
+ Hits 4047 4107 +60
- Misses 741 813 +72
- Partials 296 308 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| protected final ObjectMapper MAPPER = new ObjectMapper(); | ||
| protected ResultSet resultSet; | ||
| private final boolean removeDocumentId; | ||
| final boolean removeDocumentId; |
There was a problem hiding this comment.
Making this package private may introduce some confusion and can be error prone. Can we see if it's possible to avoid it?
… added quotes around fields
| private void addColumnToJsonNode( | ||
| ObjectNode jsonNode, String columnName, String columnType, int columnIndex) | ||
| throws SQLException { | ||
| switch (columnType.toLowerCase()) { |
There was a problem hiding this comment.
Did we face some issue, while using this one - mapValueToJsonNode? Can that be reused?
There was a problem hiding this comment.
yes we ran into a issue, that's why we created this separately
kotharironak
left a comment
kotharironak
left a comment
There was a problem hiding this comment.
Lets add few test cases.
| * different strategies for handling containment operations based on the context of the query (e.g., | ||
| * first-class fields vs. JSON fields). | ||
| */ | ||
| public interface PostgresContainsRelationalFilterParserInterface |
There was a problem hiding this comment.
Are we supporting CONTAINS already?
There was a problem hiding this comment.
Yes, some queries have that so I added it
| String flatStructureCollection = context.getFlatStructureCollectionName(); | ||
| boolean isFirstClassField = | ||
| flatStructureCollection != null | ||
| && flatStructureCollection.equals(context.getTableIdentifier().getTableName()); |
There was a problem hiding this comment.
It looks like we are applying to specific table name match. So, either table is flat table or it may contains json or non-json field. Looks like we are adding few checks, assuming we will do re-work?
There was a problem hiding this comment.
yes, we have added a jira to relook at this
3 participants
… structure
Description
Please include a summary of the change, motivation and context.
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.