Skip to content

Optimize row constructor code #15732

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kaikalur
Copy link
Contributor

@kaikalur kaikalur commented Feb 21, 2021

Test plan - Added a test in AbstractTestQueries

We now generate a single variable for each distinct primitive type and one Object type variable for all non-primitives. This helps in reducing the bytecode size dramatically. We can now process the 1250 field row that's in the test just added - that was failing before this optimization.

We also generate a single field add code per field type and a single null handling block. This is the minimal code that we can generate in our current framework.

== RELEASE NOTES ==

General Changes
* ROW constructor code is improved that should allow struct/row type with many more fields without bytecode too large error.

@kaikalur kaikalur force-pushed the optimize_row_constructor branch 2 times, most recently from 6b53e1a to bb65dec Compare February 21, 2021 17:03
@kaikalur kaikalur requested review from rongrong and highker February 22, 2021 00:34
@kaikalur kaikalur changed the title [WIP] Optimize row constructor code Optimize row constructor code Feb 22, 2021
@kaikalur kaikalur force-pushed the optimize_row_constructor branch from bb65dec to 39a90f1 Compare February 22, 2021 20:01
block.append(context.wasNull().set(constantFalse()));
Variable index = scope.createTempVariable(int.class);
BytecodeBlock loopBody = new BytecodeBlock();
BytecodeNode forLoop = new ForLoop()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add comment to each block with the corresponding java code so it would be easier to read / review? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

query.append("ROW(obj.name, obj.value, obj.version, obj.matched_content_count, " + (i % 2 == 0 ? "1" : "false") + "),");
}
query.append("null, 1) AS x,");
//query.append("ROW(" + query + "1), ");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -5662,4 +5662,19 @@ public void testMapUnionSumOverflow()
"select y, map_union_sum(x) from (select 1 y, map(array['x', 'z', 'y'], cast(array[null,30, 32760] as array<smallint>)) x " +
"union all select 1 y, map(array['x', 'y'], cast(array[1,100] as array<smallint>))x) group by y", ".*Value 32860 exceeds MAX_SHORT.*");
}

@Test
public void testSingleItemCSE()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want to just name this as testLargeRow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@kaikalur kaikalur force-pushed the optimize_row_constructor branch 4 times, most recently from 07d441f to 223f00f Compare March 6, 2021 16:53
@kaikalur kaikalur force-pushed the optimize_row_constructor branch from 223f00f to 4842650 Compare March 6, 2021 19:19
@kaikalur kaikalur requested a review from rongrong March 7, 2021 22:53
@stale
Copy link

stale bot commented Sep 6, 2021

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the task, make sure you've addressed reviewer comments, and rebase on the latest master. Thank you for your contributions!

@stale stale bot added the stale label Sep 6, 2021
@highker highker removed their request for review December 16, 2021 21:41
@stale stale bot removed the stale label Dec 16, 2021
@rohanpednekar
Copy link
Contributor

Hi @kaikalur and @rongrong, Hope all is well with you. Just checking in to see if you are still planning to work on this PR?

@prestoprobot
Copy link

@rongrong Friendly ping. Please review this PR. Thank you!

@wanglinsong wanglinsong requested review from jaystarshot, feilong-liu and a team as code owners July 6, 2024 04:32
@wanglinsong wanglinsong requested a review from presto-oss July 6, 2024 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants