ccciudatu commented on code in PR #13772:
URL: https://github.com/apache/datafusion/pull/13772#discussion_r1888925896
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -994,8 +994,34 @@ pub async fn from_substrait_rel(
)
.await
}
- _ => {
- not_impl_err!("Unsupported ReadType: {:?}",
&read.as_ref().read_type)
+ Some(ReadType::ExtensionTable(ext)) => {
Review Comment:
That's great! I like where that is going.
My goal here was to add the missing support for reading & writing extension
tables leveraging only what's available (to keep the patch as small as
possible).
But I do agree that uniform handling of Substrait extensions would make more
sense and would overcome some of the current limitations.
I think the code here is really easy to migrate to the new
`SubstraitConsumer` (and `SubstraitProducer`?) interfaces once they're
available, by just replacing the `SerializerRegistry` calls with the new
dedicated read/write methods. But the rest would be mostly unchanged by this
migration.
FWIW, once your refactoring is complete, I think there would be no place for
`SerializerRegistry` anymore, and it should be removed (or at least deprecated).
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
