[BEAM-8334] Expose Language Options for testing#9704
Conversation
apilloud
force-pushed
the
languageoptions
branch
2 times, most recently
from
3628bed to
0427598
Compare
|
R: @amaliujia |
| ResolvedStatement analyze(String sql) { | ||
| AnalyzerOptions options = initAnalyzerOptions(builder.queryParams); | ||
| AnalyzerOptions options = initAnalyzerOptions(); | ||
| for (Map.Entry<String, Value> entry : builder.queryParams.entrySet()) { |
There was a problem hiding this comment.
I am slightly against this move because the original intention is to initialize AnalyzerOptions in the same place to avoid many pieces of initializations (which will be hard to maintain). I meant "slightly" because I can see why you are moving it: it will require static parameters as input.
What do you think about the idea of having a AnalyzerOptions builder with default settings on fields?
There was a problem hiding this comment.
I agree that we should have a single place that performs common init of AnalizerOptions and I think this change still achieves that goal. The query parameters are only set in the analyze path, so I think it actually creates more confusion to move it into a separate function. I have no strong opinions and can add a function if you'd like one.
There was a problem hiding this comment.
Ok maybe just a function and we can leave more work for future.
There was a problem hiding this comment.
Done. Please take another look.
apilloud
force-pushed
the
languageoptions
branch
from
0427598 to
6579be6
Compare
|
LGTM |
2 participants
This exposes Language Options for testing.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.