Skip to content

Implement ST_Point #1106

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 9, 2025
Merged

Implement ST_Point #1106

merged 3 commits into from
May 9, 2025

Conversation

kylebarron
Copy link
Member

@kylebarron kylebarron commented May 9, 2025

It seems that the extension metadata isn't getting passed through here correctly.

I've added a test case that shows that there are two places internally where the extension metadata is correct (assertions pass; the result of return_field_from_args and the value of args.return_field in invoke_with_args). But if I collect the data into record batches with sql_df.collect(), the result schema is missing the Arrow extension metadata (assertion fails).

@timsaucer do you know if I'm doing something wrong here? Or maybe this is an edge case because I'm passing a literal into the SQL command ("SELECT ST_Point(-71.104, 42.315);")?

Elsewhere the extension metadata seemed to correctly exist on the schema of the resulting record batches. In particular, in the test testing ST_AsBinary the relevant line succeeds

let output_field = output_schema.field(0);
let output_wkb_type = output_field.try_extension_type::<WkbType>().unwrap();

@kylebarron
Copy link
Member Author

I pushed another test that shows that this works from an existing table; it just fails from a scalar apparently. @timsaucer maybe I should create an issue upstream for this?

@paleolimbot
Copy link
Contributor

Maybe apache/datafusion#15797 is a good place to track? I haven't gotten a chance to try this on scalars yet but there's definitely no ScalarValue::xx that is capable of transporting metadata at the moment.

@timsaucer
Copy link

I think I've tracked down the problem. I'll probably get to this today or tomorrow.

@kylebarron
Copy link
Member Author

Thank you! No rush on this from my side; I'm just prototyping.

@timsaucer
Copy link

This has bearing on my work as well, so getting these bugs squashed fast is important.

@kylebarron
Copy link
Member Author

I'm going to ignore the failing test on the scalar so I can merge this.

@kylebarron kylebarron enabled auto-merge (squash) May 9, 2025 21:48
@kylebarron kylebarron merged commit e26cef0 into main May 9, 2025
18 checks passed
@kylebarron kylebarron deleted the kyle/geodatafusion-st_point branch May 9, 2025 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants