Skip to content

fix(spec): accept the legacy first_not_null_value aggregate name - #794

Open
jackylee-ch wants to merge 1 commit into
apache:mainfrom
jackylee-ch:fix/legacy-aggregate-name
Open

fix(spec): accept the legacy first_not_null_value aggregate name#794
jackylee-ch wants to merge 1 commit into
apache:mainfrom
jackylee-ch:fix/legacy-aggregate-name

Conversation

@jackylee-ch

Copy link
Copy Markdown
Contributor

Java registers first_not_null_value as an SPI alias of first_non_null_value:
FieldFirstNonNullValueAggLegacyFactory sits next to the canonical factory in
META-INF/services/org.apache.paimon.factories.Factory and builds the same
aggregator. It is deliberate rather than residue — it survives the switch-case to
SPI refactor in #4335 and FirstValueAggregationITCase still creates a table with
it — but undocumented, so nothing advertises it.

Rust rejected the name at all three sites that key off it, so a table written by
Java or Flink planned fine and then failed once rows were pulled, with
ConfigInvalid: Unknown aggregate function 'first_not_null_value'. CREATE and
ALTER rejected it outright. The write path never checks the name, which is why
this only surfaces where the merge function is built.

Resolved through a single canonicalization point rather than one arm per site: the
validator/constructor guard test iterates a hardcoded name list, so a one-sided
edit would slip past it. Matching is exact, mirroring FactoryUtil#discoverFactory,
which compares identifiers with equals.

The alias stays out of the supported: ... hint — Java does not document it, so it
is read but never suggested — and errors still echo whatever the user wrote.

FieldAggregator::name is &'static str, so the aggregator reports the canonical
name. That costs no parity: the only Java message repeating the configured
identifier is the retract rejection, and retract is refused here before an
aggregator exists.

@QuakeWang QuakeWang left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

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.

2 participants