geoffreyclaude commented on PR #22000: URL: https://github.com/apache/datafusion/pull/22000#issuecomment-4369137764
> @geoffreyclaude @alamb wdyt? Thanks for the ping @adriangb! My take is that the ['RelationPlanner'](https://github.com/apache/datafusion/pull/17843) direction still makes sense here. The point was not that `TABLESAMPLE` should never be shipped by DataFusion, but that the SQL semantics should remain overridable. So I think your split makes a lot of sense: put the parquet and physical pushdown in core, but leave the SQL semantics to `RelationPlanner` though the `TableSampleSystemPlanner` you introduced. Since custom planners are inserted first (by design), downstream users can still override the built-in `SYSTEM` semantics if they need to. On adding `Sample` to `enum LogicalPlan`, I'm not fundamentally against it, but it should probably be done in a dedicated follow-up PR. Maybe that follow-up PR should also wire it up more completely with a DataFrame API, generic `SampleExec` for when pushdown isn't supported, sampled-stats as discussed in #21624... On its own, I don't see the advantage of `Sample` over `UserDefinedLogicalNodeCore`, especially given the amount of new boilerplate code it will introduce. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
