Skip to content

Data: Moving GenericFileWriterFactory to the new FormatModel API#15334

Merged
pvary merged 5 commits intoapache:mainfrom
pvary:generic_model
Feb 17, 2026
Merged

Data: Moving GenericFileWriterFactory to the new FormatModel API#15334
pvary merged 5 commits intoapache:mainfrom
pvary:generic_model

Conversation

@pvary
Copy link
Contributor

@pvary pvary commented Feb 16, 2026

Part of: #12298
Implementation of the new API: #12774

Migrating of the GenericFileWriterFactory

@github-actions github-actions bot added the data label Feb 16, 2026

/**
* @deprecated This constructor is deprecated as of version 1.11.0 and will be removed in 1.12.0.
* Position deletes that include row data are no longer supported. Use {@link
Copy link
Contributor

Choose a reason for hiding this comment

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

Position deletes that include row data are no longer supported

is this still true? i noticed its missing in the new deprecation message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the deprecation comment here because this method was introduced in the current release cycle, before we decided to deprecate the entire class. Now that the class itself is deprecated, and the replacement class doesn’t provide a way to set the position‑delete row‑data schema, the constuctor is effectively deprecated as well. However, since there’s no new equivalent method, we don’t really have a suitable place to add a dedicated deprecation message.

Copy link
Contributor

Choose a reason for hiding this comment

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

should those methods be no-ops instead? Otherwise users will hit this exception when calling those methods. Typically when we deprecate something we keep/maintain the functionality without failing immediately

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These methods were originally defined in the abstract BaseFileWriterFactory so that subclasses, such as GenericFileWriterFactory could implement them. BaseFileWriterFactory also relied on them to configure writers. The new RegistryBasedFileWriterFactory neither needs nor uses these methods, so any external calls to them would now represent a potential source of errors.

I considered three options:

  1. Remove the methods – This causes compile‑time failures when subclasses use @Override.
  2. Keep the methods but throw an exception – This fails fast but breaks callers that still rely on the old configuration path.
  3. Keep the methods as no‑ops – This avoids failures but silently ignores calls, which can mask bugs.

Both options (2) and (3) behave unpredictably if someone overrides newDataWriter, newEqualityDeleteWriter, or newPositionDeleteWriter and continues to rely on the old configure... methods: option (2) throws, option (3) silently discards the call.
After thinking through the trade‑offs, the best approach seems to be keeping the methods as-is and marking them as deprecated, leaving their behavior unchanged. This provides a clear migration signal without introducing new failure modes.

} else {
LOG.warn(
"Deprecated feature used. Position delete row schema is used to create the position delete writer.");
MetricsConfig metricsConfig =
Copy link
Contributor

Choose a reason for hiding this comment

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

should this part of the code actually mimic

public PositionDeleteWriter<T> newPositionDeleteWriter(
to maintain existing behavior? Because right now I think it's a copy of
public PositionDeleteWriter<Record> newPosDeleteWriter(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally misunderstood this comment.
Fixed now. Please check

Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

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

LGTM with one remaining comment. thanks @pvary

@pvary pvary merged commit bfcb979 into apache:main Feb 17, 2026
32 checks passed
@pvary
Copy link
Contributor Author

pvary commented Feb 17, 2026

Merged to main.
Thanks @nastra and @kevinjqliu for the reviews!

@pvary pvary deleted the generic_model branch February 17, 2026 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments