Skip to content
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

handle array columns in Trino handler #201

Merged
merged 14 commits into from
Oct 25, 2023
Merged

handle array columns in Trino handler #201

merged 14 commits into from
Oct 25, 2023

Conversation

satish-mittal
Copy link
Contributor

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:

  • 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

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Merging #201 (38cba44) into main (51f63c8) will increase coverage by 2.28%.
The diff coverage is 69.69%.

@@             Coverage Diff              @@
##               main     #201      +/-   ##
============================================
+ Coverage     70.43%   72.71%   +2.28%     
- Complexity     1047     1091      +44     
============================================
  Files            96       96              
  Lines          4762     4780      +18     
  Branches        549      550       +1     
============================================
+ Hits           3354     3476     +122     
+ Misses         1188     1080     -108     
- Partials        220      224       +4     
Flag Coverage Δ
integration 72.71% <69.69%> (+2.28%) ⬆️
unit 68.67% <69.69%> (+2.31%) ⬆️

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

Files Coverage Δ
...rino/converters/DefaultColumnRequestConverter.java 75.47% <69.69%> (+11.28%) ⬆️

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Contributor

@kotharironak kotharironak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

if (operator.equals("!=") || operator.equals("NOT IN")) {
// Support only equals and IN operator
if (operator.equals("=") || operator.equals("!=")) {
// Equals (=): CONTAINS(ip_types, 'Bot')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: changes looks fine. For readability, would it make sense to move to a function?

} else if (operator.equals("IN") || operator.equals("NOT IN")) {
// IN: CARDINALITY(ARRAY_INTERSECT(ip_types, ARRAY['Public Proxy', 'Bot'])) > 0
// NOT IN: CARDINALITY(ARRAY_INTERSECT(ip_types, ARRAY['Public Proxy', 'Bot'])) = 0
builder.append("CARDINALITY(");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: changes looks fine. For readability, would it make sense to move to a function?

@satish-mittal satish-mittal merged commit 9e5f0e8 into main Oct 25, 2023
7 of 8 checks passed
@satish-mittal satish-mittal deleted the trino_func_types branch October 25, 2023 08:30
@github-actions
Copy link

Unit Test Results

  40 files  ±  0    40 suites  ±0   13s ⏱️ -1s
303 tests +14  303 ✔️ +14  0 💤 ±0  0 ❌ ±0 

Results for commit 9e5f0e8. ± Comparison against base commit 51f63c8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants