GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — feat: Add deduplicate pushdown to clickhouse - improve materialize performance by astronautas · Pull Request #5709 · feast-dev/feast

Conversation

@astronautas

@astronautas astronautas commented Nov 3, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

#5707. We have observed significant slowdown in heavy materialization jobs. Heavy = pulling in significant period of {start_ts, end_ts}, when essentially you end up with many values to be deduplicated by Feast's compute engine. Unfortunately, Polars local engine did not show any significant speed-up. Spark is out of question due to sheer complexity of adding yet another cluster engine. We also observed nearly twice increased memory usage...

Not being a fan of altering the core, I would like to introduce a local change to Clickhouse provider to pushdown the deduplication logic to offline store. This assumes you don't have any Feast compute transformations or aggregation. I would like to just use a flag for now and not make it overcomplicated i.e. try to infer it's Pandas, if it has compute engine transformations. Keeping it simple, for now, and disabled by default.

Which issue(s) this PR fixes:

#5707

Misc

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
astronautas and others added 3 commits October 15, 2025 19:55
@astronautas @ntkathole
add pull_all_from_table_or_query for clickhouse, to align with new ma…
1752a4f 
…terialization logic (calling it)

Signed-off-by: lukas.valatka <[email protected]>
Lukas Valatka
Merge branch 'feast-dev:master' into master
a6fcf5e
@astronautas
add possibility to pushdown clickhouse deduplication to offline store
f7b7d37 
Signed-off-by: lukas.valatka <[email protected]>
@astronautas
astronautas requested a review from a team as a code owner November 3, 2025 12:56
@astronautas

astronautas commented Nov 3, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@HaoXuAI Let me know what you think. I really wouldn't like to make it more complicated than above, for now 😁 .
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@astronautas
add possibility to pushdown clickhouse deduplication to offline store
7579962 
Signed-off-by: lukas.valatka <[email protected]>
@astronautas
astronautas force-pushed the feat/add-deduplicate-pushdown-to-clickhouse branch from cbcaab6 to 7579962 Compare November 3, 2025 13:02
Lukas Valatka and others added 3 commits November 3, 2025 15:03
Lukas Valatka
Merge branch 'feast-dev:master' into feat/add-deduplicate-pushdown-to…
762de72 
…-clickhouse
@astronautas
use info, since it's an intended path too
50d84d1 
Signed-off-by: lukas.valatka <[email protected]>
@astronautas
make timestamps mandatory, filtering won't work otherwise
cfaf9a6 
Signed-off-by: lukas.valatka <[email protected]>
@astronautas
astronautas force-pushed the feat/add-deduplicate-pushdown-to-clickhouse branch from 1a442cd to cfaf9a6 Compare November 3, 2025 13:15
@astronautas astronautas changed the title feat: Add deduplicate pushdown to clickhouse feat: Add deduplicate pushdown to clickhouse - improve materialize performance Nov 3, 2025
@astronautas

astronautas commented Nov 3, 2025

Copy link
Copy Markdown
Contributor Author

@franciscojavierarceo cc
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@franciscojavierarceo

franciscojavierarceo commented Nov 3, 2025

Copy link
Copy Markdown
Member

I guess I'm curious who wouldn't want this behavior natively? Based on the other thread, it sounds like we degraded functionality and probably it would make sense to remove the pandas deduplication and make that optional.

What do you think @HaoXuAI?
astronautas reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
haoxu0
haoxu0 requested changes Nov 3, 2025
View reviewed changes

@haoxu0 haoxu0 left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks for contributing, but I think this will make it inconsistent with other offline store.
Instead of changing at the store level, I would suggest to make the materialize an option config pull_latest=true, this will explicitly let the user to be able to either pull all or pull latest.
@franciscojavierarceo The reason we change to pull_all is because pull_latest doesn't work with the transformation such as aggregation and in some cases feature_transformation as well.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
astronautas and franciscojavierarceo reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@astronautas

astronautas commented Nov 3, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for contributing, but I think this will make it inconsistent with other offline store. Instead of changing at the store level, I would suggest to make the materialize an option config pull_latest=true, this will explicitly let the user to be able to either pull all or pull latest. @franciscojavierarceo The reason we change to pull_all is because pull_latest doesn't work with the transformation such as aggregation and in some cases feature_transformation as well.

Will adjust, makes sense.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@astronautas

astronautas commented Nov 5, 2025

Copy link
Copy Markdown
Contributor Author

#5713 closing in favor of this
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@astronautas astronautas closed this Nov 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@haoxu0 haoxu0 haoxu0 requested changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@astronautas @franciscojavierarceo @haoxu0
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.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.