Re: [I] Error "entered unreachable code: NamedStructField should be rewritten in OperatorToFunction" after upgrade to 37 [datafusion]
andygrove closed issue #10181: Error "entered unreachable code: NamedStructField should be rewritten in OperatorToFunction" after upgrade to 37 URL: https://github.com/apache/datafusion/issues/10181 -- 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
Re: [I] Error "entered unreachable code: NamedStructField should be rewritten in OperatorToFunction" after upgrade to 37 [datafusion]
alamb commented on issue #10181: URL: https://github.com/apache/datafusion/issues/10181#issuecomment-2088929490 Here is a PR with a proposed new API: https://github.com/apache/datafusion/pull/10330 -- 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
Re: [I] Error "entered unreachable code: NamedStructField should be rewritten in OperatorToFunction" after upgrade to 37 [datafusion]
alamb commented on issue #10181: URL: https://github.com/apache/datafusion/issues/10181#issuecomment-2078024545 > I am not entirely sure, in delta-rs we just parse SQL strings into logical exprs and then convert it to physical exprs without doing any adjustments there. I think in 37.0.0 you'll need to update the code that converts to a physical expr to call the code in https://github.com/apache/datafusion/pull/10183 > I think it makes sense that these expr conversion are automatically done in core. Yes I agree. I will make a PR over the next few days with a proposed API -- 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
Re: [I] Error "entered unreachable code: NamedStructField should be rewritten in OperatorToFunction" after upgrade to 37 [datafusion]
ion-elgreco commented on issue #10181: URL: https://github.com/apache/datafusion/issues/10181#issuecomment-2077614189 > > @alamb just checking in, is there something we need to refactor? Or are you simplifying the API and automatically handling this within datafusion on all code paths? > > I suggest for the time being you use the pattern here (is this possible)? > > > Here is an example of how to make Expr::struct work in 37.1.0: #10183 > > For the next release (38.0.0) I was thinking would move the `create_physical_expr` function into the core (rather than an example). > > Thoughts? I am not entirely sure, in delta-rs we just parse SQL strings into logical exprs and then convert it to physical exprs without doing any adjustments there. I think it makes sense that these expr conversion are automatically done in core. Because logically between v36, 37 nothing changed in the intent of the operation. That the physical impl is different should be abstracted. -- 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
Re: [I] Error "entered unreachable code: NamedStructField should be rewritten in OperatorToFunction" after upgrade to 37 [datafusion]
alamb commented on issue #10181: URL: https://github.com/apache/datafusion/issues/10181#issuecomment-2077591431 > @alamb just checking in, is there something we need to refactor? Or are you simplifying the API and automatically handling this within datafusion on all code paths? I suggest for the time being you use the pattern here (is this possible)? > Here is an example of how to make Expr::struct work in 37.1.0: https://github.com/apache/datafusion/pull/10183 For the next release (38.0.0) I was thinking would move the `create_physical_expr` function into the core (rather than an example). Thoughts? -- 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
Re: [I] Error "entered unreachable code: NamedStructField should be rewritten in OperatorToFunction" after upgrade to 37 [datafusion]
ion-elgreco commented on issue #10181: URL: https://github.com/apache/datafusion/issues/10181#issuecomment-2076660482 @alamb just checking in, is there something we need to refactor? Or are you simplifying the API and automatically handle this within datafusion on all code paths? -- 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
Re: [I] Error "entered unreachable code: NamedStructField should be rewritten in OperatorToFunction" after upgrade to 37 [datafusion]
alamb commented on issue #10181: URL: https://github.com/apache/datafusion/issues/10181#issuecomment-2071951668 I am thinking I'll try and make a PR with such an API over the next day or two to see how it might look -- 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
Re: [I] Error "entered unreachable code: NamedStructField should be rewritten in OperatorToFunction" after upgrade to 37 [datafusion]
alamb commented on issue #10181: URL: https://github.com/apache/datafusion/issues/10181#issuecomment-2071950006 > Another way to tackle it could be to leave the concept of a GetIndexedField node at the parsing layer and pull it out of Expr (or deprecate). This would force the conversion to be done between the parse plan and the logical plan. I agree this would be clearest (basically remove `Expr::GetIndexedField` and always use a function. > I think we are talking about better API design of get_field right? I was actually thinking about an API for the usecase of "I created an `Expr `programatically and I want to convert it to a `PhyscalExpr` I can execute. We do this in InfluxDB IOx. I didn't realize that both LanceDB and Delta did the same thing Currently there is an example of how to do this in `expr_api.rs`: https://github.com/apache/datafusion/blob/711239fb13998fa5cfb1b1c865ac060613bad2c7/datafusion-examples/examples/expr_api.rs#L251-L252 Maybe it is time to "promote" that function to a real API in `datafusion-core` somewhere where it can be tested and better documented. > I think we can take reference from others. Duckdb has [struct_extract](https://duckdb.org/docs/sql/functions/nested#struct_extractstruct-entry) and [map_extract](https://duckdb.org/docs/sql/functions/nested#map_extractmap-key) Those are interesting functions -- now that we have the notion of function packages, adding better support for the Map datatype would be sweet. -- 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
Re: [I] Error "entered unreachable code: NamedStructField should be rewritten in OperatorToFunction" after upgrade to 37 [datafusion]
jayzhan211 commented on issue #10181: URL: https://github.com/apache/datafusion/issues/10181#issuecomment-2071461117 > done between the parse plan and the logical plan I had also thought about deprecate `Expr` and use functions directly in parsing phase. I think it might be a good idea. The downside of the current impl is that one needs to register function rewrite rule to convert Expr to Function. I think **Register** is better not a neccessary step for default behavior. If we have functions in parsing phase, no additional step (register rewrite) is needed. > What's the motivation for doing this at the logical level instead of doing this as part of the conversion from logical to physical? One good reason is that we don't need physical expr if we converted it to functions in logical level. I think the early we optimize, the less duplicated things we leave. -- 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
