eliaperantoni commented on issue #15276: URL: https://github.com/apache/datafusion/issues/15276#issuecomment-2739463296
Hey @jsai28, that looks good! I have some points that I'd like to hear your opinion on: 1. I think `FnCallSpans.args` should have exactly as many elements as the arguments of the function. i.e. you should always be able to do `FnCallSpans[i]` for every `arg_types[i]`. What would then happen if a `Span` is not available for the i-th argument? For example, until sqlparser is updated, `Span`s are not available for literal values. So you wouldn't have a span for the `'foo'` in `concat(var1, 'foo', var2)` I think perhaps we should use `Vec<Option<Span>>` instead? 2. What are the pros and cons of using `Span` instead of `Spans` (note the "s")? I think `Spans` is able to deal with a lack of `Span`s out of the box, so you would do `args: Vec<Spans>` and then to get the span for the i-th argument you'd do `args[i].first()`. Also `Spans` supports multiple `Span`s, not that we necessarily need it for function arguments, but who knows, maybe in the future. I also have this feeling that we should always use unless there's a good argument otherwise because it was created to be the global `Span`s container for DataFusion and will evolve to acomodate any future needs. What do you think? 3. I'm thinking maybe `ScaladUDFImpl::diagnose` should take `arg_types: ReturnTypeArgs` instead of `arg_types: &[DataType]` to be more general and futureproof. Just like `ScalarUDFImpl::return_type_from_args` is more general than `ScalarUDFImpl::return_type`. > So this essentially sets the groundwork so that a custom Diagnostic can be created for the UDFs, but does not actually implement them correct? I'm not sure what you mean with "but does not actually implement them correct". @alamb suggested using this new feature by creating a few `Diagnostic` in some of the built-in functions you see in `datafusion::functions*` so that this API is tested and provides value :) -- 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
