Skip to content

Conversation

@pjfanning
Copy link
Member

@pjfanning pjfanning commented Sep 29, 2022

Description

Remove use of java-sizeof jar. It is an old copy of Spark code. I have ported the latest Spark code to Java and inlined it.

Documentation

(Please describe user-visible changes similar to what should appear in the Drill documentation.)

Testing

(Please describe how this PR has been tested.)

@pjfanning pjfanning self-assigned this Sep 29, 2022
@pjfanning pjfanning marked this pull request as draft September 29, 2022 20:37
@pjfanning
Copy link
Member Author

@cgivre My PR uses the latest Spark code for the SizeEstimator as opposed to very old copy of the Spark code that was copied and not maintained in java-sizeof lib. The new code gives slightly higher estimates for object instance sizes. Would this be an issue?

Also, with tests - Spark and the java-sizeof lib use some Scala based tests - and they are not that easily replicated in Java. The question is whether the tests that I've added in this PR or if I need more.

@pjfanning pjfanning marked this pull request as ready for review October 5, 2022 22:46
@cgivre cgivre added code-cleanup dependencies backport-to-stable This bug fix is applicable to the latest stable release and should be considered for inclusion there labels Oct 16, 2022
Copy link
Contributor

@cgivre cgivre left a comment

Choose a reason for hiding this comment

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

LGTM +1
@pjfanning Thanks for this.

@cgivre
Copy link
Contributor

cgivre commented Oct 16, 2022

Given that this only affects the OpenTSDB plugin, I think it is fine. I've honestly never even heard of anyone actually using that with Drill.

@cgivre cgivre merged commit 1c6770c into apache:master Oct 16, 2022
@cgivre cgivre changed the title DRILL-8324: sizeof refactor DRILL-8324: Sizeof Refactor Oct 16, 2022
@pjfanning pjfanning deleted the DRILL-8324-sizeof branch October 17, 2022 16:59
kingswanwho pushed a commit to kingswanwho/drill that referenced this pull request Dec 9, 2022
kingswanwho pushed a commit to kingswanwho/drill that referenced this pull request Dec 18, 2022
jnturton pushed a commit that referenced this pull request Dec 19, 2022
ashevchuk123 pushed a commit to mapr/incubator-drill that referenced this pull request Oct 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-to-stable This bug fix is applicable to the latest stable release and should be considered for inclusion there code-cleanup dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants