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

Remove unnecessary conversion steps in PartiQLValueIonReaderBuilder #1456

Merged
merged 3 commits into from
May 7, 2024

Conversation

popematt
Copy link
Contributor

@popematt popematt commented May 6, 2024

Relevant Issues

N/A

Description

There is a lot of unnecessary computation in PartiQLValueIonReaderBuilder.build(ionElement: IonElement) as it converts from IonElement -> IonValue -> text Ion -> PartiQLValue. This PR eliminates the hop to IonValue, resulting in IonElement -> text Ion -> PartiQLValue.

// Current implementation
val reader = IonReaderBuilder.standard().build(ionElement.toIonValue(IonSystemBuilder.standard().build()))
val writer = IonTextWriterBuilder.standard().build(out)
writer.writeValues(reader) 

The IonReader in the current implementation is completely unnecessary. We can eliminate the construction and overhead of passing data through an IonReader by writing the IonValues directly to the writer.

val writer = IonTextWriterBuilder.standard().build(out)
ionElement.toIonValue(IonSystemBuilder.standard().build()).writeTo(writer)

The conversion to IonValue also adds a non-trivial amount of computation. The implementation of IonElement.toIonValue() constructs an IonWriter that will write directly to IonValue. We can take things one step further, and skip IonValue completely, which is what this PR does. I.e.:

val writer = IonTextWriterBuilder.standard().build(out)
ionElement.writeTo(writer)

Other Information

  • Updated Unreleased Section in CHANGELOG: NO

CHANGELOG says:

All notable changes to this project will be documented in this file.

Is this PR "notable"? I'll let you be the judge.

License Information

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

RCHowell
RCHowell previously approved these changes May 7, 2024
@RCHowell
Copy link
Member

RCHowell commented May 7, 2024

Need to do a ./gradlew ktlint

@RCHowell RCHowell merged commit 2a13661 into partiql:main May 7, 2024
7 checks passed
@yliuuuu
Copy link
Contributor

yliuuuu commented May 7, 2024

Good to know. Thanks for the detailed description.

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