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

Fix Relevance Fields Permissive When Fields are Missing. #158

Conversation

forestmvey
Copy link

@forestmvey forestmvey commented Nov 7, 2022

Signed-off-by: forestmvey forestv@bitquilltech.com

Description

Relevance functions that query a field should act similar to how a SELECT query works. If a field is queried that does not exist, a SemanticCheckException should be thrown.

Example Queryies

SELECT * FROM stackexchange_beer WHERE query_string([invalid], 'beer');

{'reason': 'Invalid SQL query', 'details': "can't resolve Symbol(namespace=FIELD_NAME, name=invalid) in type env", 'type': 'SemanticCheckException'}

SELECT * FROM stackexchange_beer WHERE match(invalid, 'beer');

{'reason': 'Invalid SQL query', 'details': "can't resolve Symbol(namespace=FIELD_NAME, name=invalid) in type env", 'type': 'SemanticCheckException'}

Issues Resolved

Issue: 613

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@forestmvey forestmvey force-pushed the dev-fix-permissive-relevance-fields branch from d5c0067 to 4fa357c Compare November 7, 2022 17:10
@codecov
Copy link

codecov bot commented Nov 7, 2022

Codecov Report

❗ No coverage uploaded for pull request base (integ-fix-permissive-relevance-fields@03f30e3). Click here to learn what that means.
The diff coverage is n/a.

@@                           Coverage Diff                            @@
##             integ-fix-permissive-relevance-fields     #158   +/-   ##
========================================================================
  Coverage                                         ?   98.27%           
  Complexity                                       ?     3366           
========================================================================
  Files                                            ?      327           
  Lines                                            ?     8486           
  Branches                                         ?      556           
========================================================================
  Hits                                             ?     8340           
  Misses                                           ?      142           
  Partials                                         ?        4           
Flag Coverage Δ
sql-engine 98.27% <0.00%> (?)

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

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

@forestmvey forestmvey force-pushed the dev-fix-permissive-relevance-fields branch from 4fa357c to eb1e577 Compare November 7, 2022 18:09
@forestmvey forestmvey requested a review from a team November 7, 2022 18:09
@forestmvey forestmvey force-pushed the dev-fix-permissive-relevance-fields branch from eb1e577 to 6020b4c Compare November 7, 2022 18:45
Copy link

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

Should we have integration tests?

@forestmvey
Copy link
Author

Should we have integration tests?

I have added some IT tests to cover invalid fields in my latest commit.

@forestmvey forestmvey requested review from Yury-Fridlyand, acarbonetto and a team November 9, 2022 00:25
@forestmvey forestmvey force-pushed the dev-fix-permissive-relevance-fields branch from 95e9683 to 8144b38 Compare November 9, 2022 00:26
@forestmvey forestmvey force-pushed the dev-fix-permissive-relevance-fields branch 2 times, most recently from beccd5e to a5d26cf Compare November 9, 2022 17:16
Copy link

@MaxKsyunz MaxKsyunz left a comment

Choose a reason for hiding this comment

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

LGTM.

In the upstream PR, could you reword the title? It is used to generate release notes like these.

Something like "validate field and fields parameters in relevance search functions" would work.

@forestmvey forestmvey force-pushed the dev-fix-permissive-relevance-fields branch from a5d26cf to 641f751 Compare November 10, 2022 21:06
Signed-off-by: forestmvey <forestv@bitquilltech.com>
@forestmvey forestmvey force-pushed the dev-fix-permissive-relevance-fields branch from 641f751 to 2ad64b2 Compare November 10, 2022 21:13
@forestmvey forestmvey merged commit d3700ae into integ-fix-permissive-relevance-fields Nov 10, 2022
@forestmvey forestmvey deleted the dev-fix-permissive-relevance-fields branch November 10, 2022 23:02
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.

6 participants