Skip to content

GH-38988: [Go] Expose dictionary size from DictionaryBuilder#39521

Merged
zeroshade merged 2 commits intoapache:mainfrom
ella-chao:echao/gh-38988
Jan 9, 2024
Merged

GH-38988: [Go] Expose dictionary size from DictionaryBuilder#39521
zeroshade merged 2 commits intoapache:mainfrom
ella-chao:echao/gh-38988

Conversation

@ella-chao
Copy link
Copy Markdown
Contributor

@ella-chao ella-chao commented Jan 8, 2024

Rationale for this change

Details are in #38988

What changes are included in this PR?

This adds a method to DictionaryBuilder that returns the current dictionary size.

Are these changes tested?

Updated an existing test to account for this new method.

Are there any user-facing changes?

Yes, a new method is added to DictionaryBuilder.

@ella-chao ella-chao requested a review from zeroshade as a code owner January 8, 2024 23:52
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 8, 2024

⚠️ GitHub issue #38988 has been automatically assigned in GitHub to PR creator.

Comment on lines 1007 to 1011
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should probably add this to the DictionaryBuilder interface, yea?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, done in 98b970f

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jan 9, 2024
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jan 9, 2024
@ella-chao ella-chao changed the title GH-38988: [Go] Expose dictionary size from dictionaryBuilder GH-38988: [Go] Expose dictionary size from DictionaryBuilder Jan 9, 2024
@zeroshade zeroshade merged commit 92520c6 into apache:main Jan 9, 2024
@zeroshade zeroshade removed the awaiting change review Awaiting change review label Jan 9, 2024
@ella-chao ella-chao deleted the echao/gh-38988 branch January 10, 2024 09:23
@conbench-apache-arrow
Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 92520c6.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…pache#39521)

### Rationale for this change

Details are in apache#38988

### What changes are included in this PR?

This adds a method to `DictionaryBuilder` that returns the current dictionary size.

### Are these changes tested?

Updated an existing test to account for this new method.

### Are there any user-facing changes?

Yes, a new method is added to `DictionaryBuilder`.

* Closes: apache#38988

Authored-by: Ella Chao <ella.chao@datadoghq.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
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.

[Go] Allow access to the underlying MemoTable of a dictionary builder

2 participants