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

[SEDONA-543] Fixes RS_Union_aggr throwing referenceRaster is null error when run on cluster #1364

Merged
merged 6 commits into from
Apr 26, 2024

Conversation

prantogg
Copy link
Contributor

Did you read the Contributor Guide?

Is this PR related to a JIRA ticket?

What changes were proposed in this PR?

  • In RS_Union_aggr, moves class level members like referenceRaster to data buffer.

How was this patch tested?

  • Passes existing tests

Did this PR include necessary documentation updates?

  • No, this PR does not affect any public API so no need to change the documentation.

Comment on lines 47 to 59
def serializeSampleDimension(sampleDimension: GridSampleDimension): Array[Byte] = {
val bos = new ByteArrayOutputStream()
val oos = new ObjectOutputStream(bos)
oos.writeObject(sampleDimension)
oos.flush()
bos.toByteArray
}

def deserializeSampleDimension(bytes: Array[Byte]): GridSampleDimension = {
val bis = new ByteArrayInputStream(bytes)
val ois = new ObjectInputStream(bis)
ois.readObject().asInstanceOf[GridSampleDimension]
}
Copy link
Member

Choose a reason for hiding this comment

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

We have a GridSampleDimensionSerializer implemented using Kryo. I recommend using that instead of the Java serializer.

@jiayuasu jiayuasu added the bug label Apr 25, 2024
@jiayuasu jiayuasu added this to the sedona-1.6.0 milestone Apr 25, 2024
@prantogg prantogg marked this pull request as ready for review April 26, 2024 16:51
@prantogg prantogg requested a review from jiayuasu as a code owner April 26, 2024 16:51
Copy link
Member

@jiayuasu jiayuasu Apr 26, 2024

Choose a reason for hiding this comment

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

Please don't commit this kind of empty change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I missed this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixed now.

@@ -176,4 +178,19 @@ public static GridCoverage2D deserialize(byte[] bytes) throws IOException, Class
return state.restore();
}
}

public static byte[] serializeGridSampleDimension(GridSampleDimension sampleDimension) {
Copy link
Member

Choose a reason for hiding this comment

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

This was already implemented in Sedona. Why implement it again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is as per Kristin's recommendation to implement new serialization/deserialization methods for GridSampleDimension that use kryos.get() to retrieve Kryo objects. kryos.get() avoids repeatedly initializing new Kryo instances.
I should have used the existing GridSampleDimensionSerializer for read and write into output buffer. This is added now.

@prantogg prantogg requested a review from jiayuasu April 26, 2024 20:06
@jiayuasu jiayuasu merged commit a7ad5f0 into apache:master Apr 26, 2024
38 of 50 checks passed
jiayuasu pushed a commit that referenced this pull request Apr 28, 2024
…or when run on cluster (#1364)

* Init: move class level members to data buffer

* move sampleDimension serde to Serde.java

* update serde for sampleDimensions

* Add checks for index

* Undo typo

* add custom GridSampleDimensionSerializer
jiayuasu added a commit that referenced this pull request Apr 28, 2024
…null error when run on cluster (#1364)"

This reverts commit 2fce212.
jiayuasu added a commit that referenced this pull request Apr 28, 2024
…ster is null error when run on cluster (#1364)""

This reverts commit 57a2d37.
jiayuasu added a commit that referenced this pull request Apr 28, 2024
…erenceRaster is null error when run on cluster (#1364)"""

This reverts commit 0ab9304.
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.

None yet

3 participants