Skip to content

Conversation

shangm2
Copy link

@shangm2 shangm2 commented Jun 4, 2025


@CodecThriftType
public static ThriftType getThriftType()
public static ThriftType getThriftType(ThriftCatalog catalog)
Copy link
Author

Choose a reason for hiding this comment

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

This allows us to retrieve metadata for other classes.

position = 0;
}

public byte[] getBytes()
Copy link
Author

Choose a reason for hiding this comment

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

We need to retrieve the bytes because our serde is general and it returns bytes https://github.com/prestodb/presto/pull/25242/files#r2127315310

Copy link
Member

Choose a reason for hiding this comment

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

Consider creating a separate TMemoryBuffer implementation based on the ByteArrayOutputStream with only write method available. Consider using the more efficient ByteArrayOutputStream implementation from Guava.


@CodecThriftType
public static ThriftType getThriftType()
public static ThriftType getThriftType(ThriftCatalog catalog)
Copy link
Member

Choose a reason for hiding this comment

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

Please extract the change adding the ThriftCatalog catalog into a separate commit

Copy link
Author

@shangm2 shangm2 Jun 11, 2025

Choose a reason for hiding this comment

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

I was trying to use the passed-in catalog to retrieve the metadata for fields within Split because I was manually building the metadata for split. I have updated the code to use annotation for split and custom codec for complicated field within it so we dont need to catalog anymore. My apology for the confusion.


@CodecThriftType
public static ThriftType getThriftType()
public static ThriftType getThriftType(ThriftCatalog catalog)
Copy link
Member

Choose a reason for hiding this comment

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

Could you please explain what problem are you trying to solve? Why is it necessary to have access to ThriftCatalog to define a thrift type? Ideally this method should be pure constant / static.

Copy link
Author

@shangm2 shangm2 Jun 11, 2025

Choose a reason for hiding this comment

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

I was trying to use the passed-in catalog to retrieve the metadata for fields within Split because I was manually building the metadata for split. I have updated the code to use annotation for split and custom codec for complicated field within it so we dont need to catalog anymore. My apology for the confusion.

position = 0;
}

public byte[] getBytes()
Copy link
Member

Choose a reason for hiding this comment

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

Consider creating a separate TMemoryBuffer implementation based on the ByteArrayOutputStream with only write method available. Consider using the more efficient ByteArrayOutputStream implementation from Guava.

@shangm2 shangm2 force-pushed the getBytes branch 3 times, most recently from aaa0ee6 to 88487ee Compare June 11, 2025 17:21
@arhimondr arhimondr merged commit 1c51d99 into prestodb:master Jul 5, 2025
3 checks passed
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.

2 participants