polyzos commented on PR #3398:
URL: https://github.com/apache/fluss/pull/3398#issuecomment-4629197893
Thanks for the clean, well-documented PR @Prajwal-banakar.
The code is in good shape and the rationale comments make it easy to follow.
I have a few suggestions before this is ready to merge, mostly one important
test gap and one design question.
### 1. Add an end-to-end SQL ITCase (most important)
The current tests call `accumulate()`/`getValue()` directly, which bypasses
the Flink planner. The riskiest part of this feature is the
`@FunctionHint(accumulator = @DataTypeHint("RAW"))` type inference and the
custom accumulator serializer flowing through real codegen, **plus** the new
catalog registration resolving the function isn't exercised yet.
RAW-accumulator UDFs commonly pass unit tests but break at planner time. Could
we add an ITCase (the existing `AbstractRbAggFunctionITCase` from #3319 is a
natural home) that runs something like:
```sql
USE CATALOG fluss_catalog;
SELECT rb_build_agg(id), rb_or_agg(b), rb_and_agg(b) FROM t GROUP BY k;
```
through a `TableEnvironment`? That would directly validate the "register via
FlussCatalog" claim in the PR title.
### 2. Built-in registrationion
I think hardcoding a `name -> className` map inside the general-purpose
`FlinkCatalog` means the functions get reported under *every* database via
`listFunctions(dbName)`. Since we only have these functions i think its safe
and this list won't keep growing, however I would like more people's thoughts
just in case.. @platinumhamburg @wuchong WDYT?
I left a few more minor comments/
Once the end-to-end ITCase is added (1) and we align on the registration
approach (2), this should be a quick approve. Thanks again!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
