feat: Add support for postgres - Part 1 #153
Conversation
Codecov Report
@@ Coverage Diff @@
## main #153 +/- ##
=============================================
- Coverage 82.66% 71.73% -10.93%
- Complexity 630 761 +131
=============================================
Files 65 82 +17
Lines 2359 3361 +1002
Branches 243 374 +131
=============================================
+ Hits 1950 2411 +461
- Misses 312 818 +506
- Partials 97 132 +35
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryFunctionConstants.java
Outdated
Show resolved
Hide resolved
...ice-impl/src/main/java/org/hypertrace/core/query/service/postgres/PostgresClientFactory.java
Outdated
Show resolved
Hide resolved
...vice-impl/src/main/java/org/hypertrace/core/query/service/postgres/PostgresMapConverter.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -18,4 +18,5 @@ public interface QueryFunctionConstants { | |||
| String QUERY_FUNCTION_STRINGEQUALS = "STRINGEQUALS"; | |||
| String QUERY_FUNCTION_CONDITIONAL = "CONDITIONAL"; | |||
| String QUERY_FUNCTION_CONCAT_OR_NULL = "CONCATORNULL"; | |||
| String DATA_TIME_CONVERT = "DATETIMECONVERT"; | |||
There was a problem hiding this comment.
Q: we will define this function at Postgres side?
There was a problem hiding this comment.
Yes, we will define it on postgres side
query-service-impl/src/test/resources/service_request_handler.conf
Outdated
Show resolved
Hide resolved
query-service-impl/src/test/resources/postgres_request_handler.conf
Outdated
Show resolved
Hide resolved
|
|
||
| assertSQLQuery( | ||
| builder.build(), | ||
| "Select distinct encode(span_id, 'hex'), span_name, service_name FROM public.\"span-event-view\" " |
There was a problem hiding this comment.
In the case of multiple selections, do distinct apply to the first field?
There was a problem hiding this comment.
Distinct will apply to selection across multiple columns, and not just the first one.
| void testSQLiWithStringValueFilter() { | ||
| QueryRequest queryRequest = | ||
| buildSimpleQueryWithFilter( | ||
| createEqualsFilter("Span.displaySpanName", "GET /login' OR tenant_id = 'tenant2")); |
There was a problem hiding this comment.
What is the difference with testQueryWithStringFilter? It compares entire string, right?
There was a problem hiding this comment.
Is this to do with PrepareStatement?
There was a problem hiding this comment.
Copied from pinot. Need to check this part.
.../java/org/hypertrace/core/query/service/postgres/QueryRequestToPostgresSQLConverterTest.java
Outdated
Show resolved
Hide resolved
.../java/org/hypertrace/core/query/service/postgres/QueryRequestToPostgresSQLConverterTest.java
Outdated
Show resolved
Hide resolved
.../java/org/hypertrace/core/query/service/postgres/QueryRequestToPostgresSQLConverterTest.java
Outdated
Show resolved
Hide resolved
| + TENANT_ID | ||
| + "' " | ||
| + "and ( start_time_millis > 1570658506605 and end_time_millis < 1570744906673 )" | ||
| + " group by service_name, span_name order by service_name, avg(duration_millis) desc , count(*) desc limit 20", |
There was a problem hiding this comment.
SQL support alias in ORDER by clause. So, as follow-up, we should replace it with alias.
count(*) as count
avg(duration_millis) as avg_duration_millis
So order_by will become,
order by service_name, avg_duration_millis desc , count desc
There was a problem hiding this comment.
In my to-do list
There was a problem hiding this comment.
Will handle alias related changes in separate PR
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
|
||
| assertSQLQuery( | ||
| queryRequest, | ||
| "select count(distinct encode(span_id, 'hex')) FROM public.\"span-event-view\"" |
There was a problem hiding this comment.
we are missing alias distinctcount_span_id in the final query, would that be an issue?
There was a problem hiding this comment.
Will handle alias related changes in separate PR
|
|
||
| assertSQLQuery( | ||
| queryRequest, | ||
| "select encode(span_id, 'hex'), count(distinct encode(span_id, 'hex')) FROM public.\"span-event-view\"" |
There was a problem hiding this comment.
missing alias distinctcount_span_id in the final query
There was a problem hiding this comment.
Will handle alias related changes in separate PR
| + TENANT_ID | ||
| + "' " | ||
| + "and ( start_time_millis > 1570658506605 and end_time_millis < 1570744906673 )" | ||
| + " group by span_id order by count(distinct span_id) limit 15", |
There was a problem hiding this comment.
Note: we should use alias in order by. Like in this example, we might have two eval for count(distinct span_id) in order clause and count(distinct encode(span_id, 'hex')) in select clause as expressions are different. However, we can confirm this by explain_plan.
There was a problem hiding this comment.
Will handle alias related changes in separate PR
|
|
||
| assertSQLQuery( | ||
| builder.build(), | ||
| "SELECT span_name FROM public.\"span-event-view\" WHERE " |
There was a problem hiding this comment.
Q: Shouldn't such a query be an exception? Are we facing some issues with PrepareStatement?
There was a problem hiding this comment.
Need to check this test. Copied from Pinot.
| + " = '" | ||
| + TENANT_ID | ||
| + "' " | ||
| + "AND span_id like decode('042e5523ff6b2506', 'hex')", |
There was a problem hiding this comment.
The encode function is used only with bytes fields, right? So, if the field would have service_name, query would have been service_name like 'frontend' or ``service_name like frontend`?
There was a problem hiding this comment.
Yes, encode/decode is only for bytes field
| Expression tag = createStringArrayLiteralValueExpression(List.of("FLAGS", "0")); | ||
| Filter likeFilter = | ||
| Filter.newBuilder() | ||
| .setOperator(Operator.CONTAINS_KEYVALUE) |
There was a problem hiding this comment.
Note: CONTAINS_KEYVALUE is deprecated.
| + " = '" | ||
| + TENANT_ID | ||
| + "' " | ||
| + "AND ( parent_span_id != '' ) limit 5", |
There was a problem hiding this comment.
Q: is '' default for bytea?
There was a problem hiding this comment.
I have checked that it works. However IS NULL or IS NOT NULL is a better check. Will add it in follow up PR.
|
|
||
| assertSQLQuery( | ||
| queryRequest, | ||
| "select PERCENTILETDIGEST99(duration_millis) FROM public.\"span-event-view\"" |
There was a problem hiding this comment.
This will depend on the configuration of the handler, right?
| // Create a Postgres Client. | ||
| public static PostgresClient createPostgresClient(String postgresCluster, String path) | ||
| throws SQLException { | ||
| if (!get().containsClient(postgresCluster)) { |
There was a problem hiding this comment.
Q: we are creating a client per handler as per RequestHandler build, right?
There was a problem hiding this comment.
Yes, one connection per handler
...pl/src/main/java/org/hypertrace/core/query/service/postgres/PostgresBasedRequestHandler.java
Outdated
Show resolved
Hide resolved
| * | ||
| * <p>A query can usually be handled by Postgres handler if the Postgres view of this handler has | ||
| * all the columns that are referenced in the incoming query. If the Postgres view is a filtered | ||
| * view on some view column filters, the incoming query has to have those filters to match the |
There was a problem hiding this comment.
If the Postgres view is a filtered view on some view column filters, the incoming query has to have those filters to match the view.
Didn't get fully. It should be all referenced columns including selection, where, group_by, order_by, etc, right?
There was a problem hiding this comment.
Just saw viewDefinitionSupportsFilter, I think, it is for additional configuration support.
| expressionValues.add(String.valueOf(value.getInt())); | ||
| break; | ||
| case INT_ARRAY: | ||
| expressionValues.addAll( |
There was a problem hiding this comment.
Q:are we treating INT_ARRAY as a set in other impl too?
There was a problem hiding this comment.
Yes, same as Pinot
...pl/src/main/java/org/hypertrace/core/query/service/postgres/PostgresBasedRequestHandler.java
Outdated
Show resolved
Hide resolved
...pl/src/main/java/org/hypertrace/core/query/service/postgres/PostgresBasedRequestHandler.java
Outdated
Show resolved
Hide resolved
...ce-impl/src/main/java/org/hypertrace/core/query/service/postgres/PostgresResultAnalyzer.java
Outdated
Show resolved
Hide resolved
| * query types we might be able to get rid of this in the future and have a single flow to parse the | ||
| * Postgres Response. | ||
| */ | ||
| public interface ResultSetTypePredicateProvider { |
There was a problem hiding this comment.
I read above comment, but guess, need to check with you offline to understand this one.
There was a problem hiding this comment.
Let's fix it as part of integration tests
...y-service-impl/src/main/java/org/hypertrace/core/query/service/postgres/TableDefinition.java
Outdated
Show resolved
Hide resolved
...in/java/org/hypertrace/core/query/service/postgres/converters/PostgresFunctionConverter.java
Outdated
Show resolved
Hide resolved
...in/java/org/hypertrace/core/query/service/postgres/converters/PostgresFunctionConverter.java
Outdated
Show resolved
Hide resolved
| case QUERY_FUNCTION_CONDITIONAL: | ||
| throw new UnsupportedOperationException("Unsupported function " + function); | ||
| default: | ||
| // TODO remove once postgres-specific logic removed from gateway - this normalization |
There was a problem hiding this comment.
pinot specific, right?
There was a problem hiding this comment.
Need changes in gateway before we can change here
| return String.format( | ||
| "%s(%s, %d)", | ||
| config.getDataTimeConvertFunction(), | ||
| argumentConverter.apply(argumentsList.get(0)), |
There was a problem hiding this comment.
curious Q: what would the second argument mean here?
...in/java/org/hypertrace/core/query/service/postgres/converters/PostgresFunctionConverter.java
Show resolved
Hide resolved
...main/java/org/hypertrace/core/query/service/postgres/QueryRequestToPostgresSQLConverter.java
Outdated
Show resolved
Hide resolved
| delim = ", "; | ||
| } | ||
|
|
||
| pqlBuilder.append(" FROM public.\"").append(tableDefinition.getTableName()).append("\""); |
There was a problem hiding this comment.
Do you think that we will need config for namespacepublic? For now, I think, it will be fine. lets see with our final setup.
There was a problem hiding this comment.
Yes, we should take it from config
| pqlBuilder.append(delim); | ||
| pqlBuilder.append( | ||
| columnRequestConverter.convertSelectClause(expr, paramsBuilder, executionContext)); | ||
| delim = ", "; |
There was a problem hiding this comment.
nit: delim mgmt is a bit tricky, we can first collect all select clause transformations in the stream, and join at the end. we can do this as follow-up.
There was a problem hiding this comment.
Will be done in follow up PR
...main/java/org/hypertrace/core/query/service/postgres/QueryRequestToPostgresSQLConverter.java
Outdated
Show resolved
Hide resolved
...ava/org/hypertrace/core/query/service/postgres/converters/DefaultColumnRequestConverter.java
Outdated
Show resolved
Hide resolved
| throws SQLException { | ||
| List<Builder> rowBuilderList = new ArrayList<>(); | ||
| if (resultSet.next()) { | ||
| // Postgres has different Response format for selection and aggregation/group by query. |
There was a problem hiding this comment.
I think this is the case with pinot. Postgres will always return as a list of columns in response, right?
There was a problem hiding this comment.
I think we need a single parse function in Postgres.
There was a problem hiding this comment.
That part is correct. Will fix it in follow up PR.
...pl/src/main/java/org/hypertrace/core/query/service/postgres/PostgresBasedRequestHandler.java
Outdated
Show resolved
Hide resolved
...pl/src/main/java/org/hypertrace/core/query/service/postgres/PostgresBasedRequestHandler.java
Outdated
Show resolved
Hide resolved
...pl/src/main/java/org/hypertrace/core/query/service/postgres/PostgresBasedRequestHandler.java
Outdated
Show resolved
Hide resolved
...pl/src/main/java/org/hypertrace/core/query/service/postgres/PostgresBasedRequestHandler.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -520,28 +520,27 @@ private void handleSelection( | |||
| } while (resultSet.next()); | |||
| } | |||
|
|
|||
| // TODO - Need to validate and fix this function | |||
There was a problem hiding this comment.
Yes, we will need only single parse function in Postgres impl
There was a problem hiding this comment.
As part of follow up
...in/java/org/hypertrace/core/query/service/postgres/converters/PostgresFunctionConverter.java
Outdated
Show resolved
Hide resolved
...y-service-impl/src/main/java/org/hypertrace/core/query/service/postgres/TableDefinition.java
Outdated
Show resolved
Hide resolved
| @@ -1,7 +1,6 @@ | |||
| { | |||
| name = raw-service-view-events-handler | |||
| type = pinot | |||
| clientConfig = broker | |||
There was a problem hiding this comment.
Note: In real config, we will have clientConfig with connectionString of postgres URL. we will need a way to accept password via config too.
There was a problem hiding this comment.
Need to redefine it as granular as we want
This comment has been minimized.
This comment has been minimized.
| } | ||
| } | ||
| String physName = tableDefinition.getPhysicalColumnName(logicalName); | ||
| logicalNameToPhysicalNameIndex.put(logicalName, physicalNameColIndexMap.get(physName)); |
There was a problem hiding this comment.
can physicalNameColIndexMap.get(physName) be null? Do we need to check for contains? Ideally, it shouldn't be, right?
There was a problem hiding this comment.
Shouldn't be given it is based on selections
This PR,