[1/2] Resolve functions from the catalog #1584
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CROSS-ENGINE Conformance Report ❌
Testing DetailsResult Details
Now Failing Tests ❌The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact. Now Passing TestsThe following 1 test(s) were previously FAILING in BASE but are now PASSING in TARGET. Before merging, confirm they are intended to pass: Click here to see
CROSS-COMMIT-EVAL Conformance Report ✅
Testing DetailsResult Details
CROSS-COMMIT-LEGACY Conformance Report ✅
Testing DetailsResult Details
|
RCHowell
force-pushed
the
v1-fn-path
branch
from
c7da0b2 to
408cdcb
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## v1 #1584 +/- ##
=====================================
Coverage ? 77.26%
Complexity ? 2470
=====================================
Files ? 253
Lines ? 18571
Branches ? 3491
=====================================
Hits ? 14349
Misses ? 3196
Partials ? 1026
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
alancai98
approved these changes
Sep 19, 2024
Comment on lines
+67
to
+72
| * Returns a collection of scalar functions in this catalog with the given name, or an empty list if none. | ||
| */ | ||
| public fun getFunctions(session: Session, name: String): Collection<Function> = SqlFnProvider.lookupFn(name) | ||
|
|
||
| /** | ||
| * Returns a collection of aggregation functions in this catalog with the given name, or an empty list if none. |
There was a problem hiding this comment.
nit: collection vs list
Suggested change
| * Returns a collection of scalar functions in this catalog with the given name, or an empty list if none. | |
| */ | |
| public fun getFunctions(session: Session, name: String): Collection<Function> = SqlFnProvider.lookupFn(name) | |
| /** | |
| * Returns a collection of aggregation functions in this catalog with the given name, or an empty list if none. | |
| * Returns a collection of scalar functions in this catalog with the given name, or an empty collection if none. | |
| */ | |
| public fun getFunctions(session: Session, name: String): Collection<Function> = SqlFnProvider.lookupFn(name) | |
| /** | |
| * Returns a collection of aggregation functions in this catalog with the given name, or an empty collection if none. |
Comment on lines
+25
to
+28
| /** | ||
| * TODO REMOVE ME ?? | ||
| */ | ||
| public fun getSpecific(): String |
There was a problem hiding this comment.
Will this be kept for v1 or is it just used as part of the refactoring process?
There was a problem hiding this comment.
Part of refactoring, removed in next PR
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relevant Issues
#1496
Description
This Part 1 of 2 – the second part will remove SqlFnProvider entirely.
This PR removes direct calls to the SqlFnProvider for lookup ONLY as a piecewise step towards implementing the PartiQL-PATH in which we search the session PATH for the function names. The implementations are still provided via lookup by specific.
Names
The following changes are incremental,
I have a follow-up issue for recent discussion points, #1585.
Other Information
and Code Style Guidelines? YES
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.