Skip to content

feat: add Dataframe.cache() factory (no planner handling)#1420

Merged
milenkovicm merged 6 commits intoapache:mainfrom
killzoner:issue-1395
Feb 18, 2026
Merged

feat: add Dataframe.cache() factory (no planner handling)#1420
milenkovicm merged 6 commits intoapache:mainfrom
killzoner:issue-1395

Conversation

@killzoner
Copy link
Copy Markdown
Contributor

@killzoner killzoner commented Jan 28, 2026 •
edited
Loading

Which issue does this PR close?

Closes #1395 (partially if we want to expand on future configurable cache later).

Rationale for this change

Disable explicitly cache() method with good user level error rather than relying on current panic

What changes are included in this PR?

Are there any user-facing changes?

User level error instead of panic

killzoner
killzoner commented Jan 28, 2026
View reviewed changes
Comment thread ballista/client/tests/context_unsupported.rs Outdated
@killzoner killzoner marked this pull request as ready for review January 28, 2026 19:01
milenkovicm
milenkovicm reviewed Jan 28, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@milenkovicm milenkovicm left a comment

Choose a reason for hiding this comment

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

thanks @killzoner
I have one suggestion

Comment thread ballista/core/src/extension.rs
@killzoner killzoner marked this pull request as draft January 28, 2026 19:38
@milenkovicm
Copy link
Copy Markdown
Contributor

milenkovicm commented Jan 28, 2026

no need to implement anything on scheduler side at the moment

@killzoner killzoner force-pushed the issue-1395 branch 5 times, most recently from 7d48acc to e76826a Compare February 3, 2026 11:09
@milenkovicm
Copy link
Copy Markdown
Contributor

milenkovicm commented Feb 4, 2026

You need to serialise logical plan extension, showcase project https://github.com/milenkovicm/ballista_extensions

@killzoner
Copy link
Copy Markdown
Contributor Author

killzoner commented Feb 4, 2026

You need to serialise logical plan extension, showcase project https://github.com/milenkovicm/ballista_extensions

Thanks, I was looking at this part and I was not sure I needed to update ballista proto, this gives more context 🙏

@killzoner killzoner force-pushed the issue-1395 branch 2 times, most recently from fafed16 to e313d85 Compare February 9, 2026 19:32
@killzoner killzoner marked this pull request as ready for review February 10, 2026 17:17
@killzoner
Copy link
Copy Markdown
Contributor Author

killzoner commented Feb 10, 2026 •
edited
Loading

I will work on adapting the datafusion example apache/datafusion#19139 with ballista custom query planner once this one is done

killzoner
killzoner commented Feb 10, 2026
View reviewed changes
Comment thread ballista/client/tests/context_unsupported.rs Outdated
killzoner
killzoner commented Feb 10, 2026
View reviewed changes
Comment thread ballista/core/proto/ballista.proto Outdated
@milenkovicm
Copy link
Copy Markdown
Contributor

milenkovicm commented Feb 10, 2026

I will work on adapting the datafusion example apache/datafusion#19139 with ballista custom query planner once this one is done

no need to provide ballista planner at the moment, its ok to fail

@killzoner killzoner force-pushed the issue-1395 branch 2 times, most recently from 4674dc9 to 903d000 Compare February 11, 2026 09:13
@killzoner killzoner changed the title feat: disable DataFrame.cache() for ballista feat: add cache() factory Feb 11, 2026
@killzoner killzoner changed the title feat: add cache() factory feat: add Dataframe.cache() factory Feb 11, 2026
@killzoner killzoner changed the title feat: add Dataframe.cache() factory feat: add Dataframe.cache() factory (no planner handling) Feb 11, 2026
milenkovicm
milenkovicm reviewed Feb 11, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@milenkovicm milenkovicm left a comment

Choose a reason for hiding this comment

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

thansk @killzoner few comments and one idea.

what do you think if we add configuration option which would make cache operation a no op instead of throwing exception?

Comment thread ballista/core/proto/ballista.proto
Comment thread ballista/core/src/extension.rs
Comment thread ballista/core/src/extension.rs Outdated
Comment thread ballista/core/src/extension.rs Outdated
@killzoner
Copy link
Copy Markdown
Contributor Author

killzoner commented Feb 11, 2026 •
edited
Loading

thansk @killzoner few comments and one idea.

what do you think if we add configuration option which would make cache operation a no op instead of throwing exception?

Thanks for the review! All the points make sense to me, putting this to draft and addressing them in the future days

@killzoner killzoner marked this pull request as draft February 11, 2026 22:54
@killzoner killzoner marked this pull request as ready for review February 16, 2026 17:09
@killzoner
Copy link
Copy Markdown
Contributor Author

killzoner commented Feb 16, 2026

Updated, this item is still pending though #1420 (comment)

@killzoner killzoner force-pushed the issue-1395 branch 2 times, most recently from 7e8698a to 90b862a Compare February 16, 2026 18:39
@milenkovicm
Copy link
Copy Markdown
Contributor

milenkovicm commented Feb 17, 2026

will review it asap, thanks @killzoner

milenkovicm
milenkovicm approved these changes Feb 17, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@milenkovicm milenkovicm left a comment

Choose a reason for hiding this comment

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

thanks @killzoner just minor comments about tests, otherwise it looks good

Comment thread ballista/client/tests/context_unsupported.rs
Comment thread ballista/client/tests/context_unsupported.rs Outdated
milenkovicm
milenkovicm reviewed Feb 17, 2026
View reviewed changes
Comment thread ballista/client/tests/context_unsupported.rs Outdated
@killzoner
Copy link
Copy Markdown
Contributor Author

killzoner commented Feb 18, 2026 •
edited
Loading

thanks @killzoner just minor comments about tests, otherwise it looks good

Thank you ! I guess there is only #1420 (comment) remaining, but I'm not sure on what to do given the trait API

@milenkovicm
Copy link
Copy Markdown
Contributor

milenkovicm commented Feb 18, 2026

thanks @killzoner just minor comments about tests, otherwise it looks good

Thank you ! I guess there is only #1420 (comment) remaining, but I'm not sure on what to do given the trait API

thats ok, we can leave it as it is

killzoner
killzoner commented Feb 18, 2026
View reviewed changes
Comment thread ballista/core/src/serde/mod.rs Outdated
milenkovicm
milenkovicm reviewed Feb 18, 2026
View reviewed changes
Comment thread ballista/core/src/serde/mod.rs Outdated
DataFusionError::Internal(format!(
"Could not deserialize BallistaLogicalPlanNode: {e}"
))
})?;
Copy link
Copy Markdown
Contributor

@milenkovicm milenkovicm Feb 18, 2026

Choose a reason for hiding this comment

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

yes

@milenkovicm
Copy link
Copy Markdown
Contributor

milenkovicm commented Feb 18, 2026

merging this, thanks @killzoner

@milenkovicm milenkovicm merged commit 771e280 into apache:main Feb 18, 2026
15 checks passed
@killzoner killzoner deleted the issue-1395 branch February 18, 2026 13:33
@claude claude Bot mentioned this pull request Mar 11, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@milenkovicm milenkovicm milenkovicm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug: Disable DataFrame.cache() for ballista

2 participants

@killzoner @milenkovicm