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

core/statement.cpp : Modified to support complex queries #1157

Closed
wants to merge 2 commits into from

Conversation

JoshuaVThomas
Copy link

@JoshuaVThomas JoshuaVThomas commented Jul 23, 2024

Added a part where describe is called again if numcols doesn't get populated before execution of query.

Ref : #1151

@vadz
Copy link
Member

vadz commented Jul 23, 2024

Thanks, but it would be possible to add a test case that fails now but would work with this change? Otherwise this risks getting broken again in the future.

@vadz
Copy link
Member

vadz commented Aug 19, 2024

Sorry, I really can't merge without understanding what exactly this fixes.

@JoshuaVThomas
Copy link
Author

Hi @vadz I've added the test case where this failed but succeeds now.

@vadz
Copy link
Member

vadz commented Oct 9, 2024

Thank you, but I'll need some help with understanding what exactly happens with this query... Any extra explanations would be welcome!

@JoshuaVThomas
Copy link
Author

This is a sample query which has a CURSOR declared. It's aim is to execute a complex query at ones instead of set by set operation. Since it's a complex query it takes some time to execute so that the numcols doesn't get set. So the code has been modified to address this.

@vadz
Copy link
Member

vadz commented Nov 17, 2024

Sorry for the delay, I've finally looked at it and I couldn't find any better way to fix this than to apply almost this change, please see #1182 which I've created to replace this one.

The main difference is that I don't understand how did it work for you, and even in the CI build, without calling define_for_row() after the 2nd describe(). In my local testing this just crashed because intosForRow_ elements were never initialized and, again, I just don't see how could it work without it.

Anyhow, if you don't have any objections to calling define_for_row() too, I'll merge the other PR soon.

Thanks again for the fix!

@vadz vadz closed this Nov 17, 2024
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