Skip to content
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

Aggregate relations do not yet support output mapping changes #121

Closed
mortbopet opened this issue Sep 9, 2024 · 5 comments
Closed

Aggregate relations do not yet support output mapping changes #121

mortbopet opened this issue Sep 9, 2024 · 5 comments

Comments

@mortbopet
Copy link
Contributor

Unfortunately, my queries seem to hit the Aggregate relations do not yet support output mapping changes error. However, for my given usecase, sufficient information is retained/parsed if i just comment out the following lines:

if (!relationData->outputFieldReferences.empty()) {
errorListener_->addError(
"Aggregate relations do not yet support output mapping changes.");
return;
}

Now that may be bad... but again, works for my usecase. My question is therefore - is there a way to opt-in to this feature, or make the error recoverable, s.t. users (like me) can use upstream substrait-cpp?

@EpsilonPrime
Copy link
Member

I put that check in there because it's important to handle (it affects the rest of the plan) but I didn't see any cases where folks were using it (in fact, some systems don't even support emits in aggregations yet). I'll make a stab at adding this missing feature.

@mortbopet
Copy link
Contributor Author

I put that check in there because it's important to handle (it affects the rest of the plan) but I didn't see any cases where folks were using it (in fact, some systems don't even support emits in aggregations yet). I'll make a stab at adding this missing feature.

Much appreciated! 🙏

@EpsilonPrime
Copy link
Member

Progress update: I have binary to text conversion working correctly but there are one or two small bugs in the text to binary parsing that I need to fix. Shouldn't be more than another day.

EpsilonPrime added a commit that referenced this issue Oct 1, 2024
This adds support for emit in aggregations where there is no more than
one grouping section.

This addresses #121 .
@EpsilonPrime
Copy link
Member

Please let me know if I missed anything.

@mortbopet
Copy link
Contributor Author

So far looks like the fix is working for my use-case. Thank you very much for looking into this!

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

No branches or pull requests

2 participants