Skip to content

RowBasedKeySerde should use empty dictionary in constructor#6256

Merged
gianm merged 1 commit intoapache:masterfrom
gaodayue:fix-dict-memoty
Aug 29, 2018
Merged

RowBasedKeySerde should use empty dictionary in constructor#6256
gianm merged 1 commit intoapache:masterfrom
gaodayue:fix-dict-memoty

Conversation

@gaodayue
Copy link
Copy Markdown
Contributor

Fixes #6255

Copy link
Copy Markdown
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

LGTM. Could you please add a comment about why the map is starting out empty (avoiding allocating too much when it's not needed)? Otherwise, a future contributor might not realize it.

@jihoonson
Copy link
Copy Markdown
Contributor

Hi @gaodayue, I left a comment on #6255.

Copy link
Copy Markdown
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

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

I've checked more around RowBasedKeySerde and noticed that it's initialized to 10000 no matter whether spilling is enabled or not in ConcurrentGrouper. Also, it's always initialized no matter what the column type is even though it's being used for only string columns. Probably we can optimize this further, but it looks a different issue. This patch looks good to me. Thanks @gaodayue!

@clintropolis
Copy link
Copy Markdown
Member

We should probably backport this to 0.12.3

@gianm gianm added this to the 0.12.3 milestone Aug 29, 2018
@gianm gianm merged commit fcf8c8d into apache:master Aug 29, 2018
gianm pushed a commit to gianm/druid that referenced this pull request Aug 29, 2018
gianm pushed a commit to implydata/druid-public that referenced this pull request Aug 29, 2018
@gaodayue
Copy link
Copy Markdown
Contributor Author

Could you please add a comment about why the map is starting out empty (avoiding allocating too much when it's not needed)? Otherwise, a future contributor might not realize it.

Thank you @gianm . It's a good advice, but the PR is merged. Maybe we can add it in future optimization.

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.

5 participants