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

RCORE-7544 Optimize EQ search for a single interned string #7596

Open
wants to merge 6 commits into
base: fsa/string-interning
Choose a base branch
from

Conversation

finnschiermer
Copy link
Contributor

First half (EQ part) of #7544

@cla-bot cla-bot bot added the cla: yes label Apr 16, 2024
@finnschiermer finnschiermer changed the title Optimize EQ search for a single interned string RCORE-7544 Optimize EQ search for a single interned string Apr 16, 2024
Copy link
Member

@nicola-cab nicola-cab left a comment

Choose a reason for hiding this comment

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

Looks good, I have just a bunch of no blocking comments.

@@ -2931,27 +2931,9 @@ DisableReplication::~DisableReplication()
m_tr.initialize_replication();
}

StringInterner* DB::get_string_interner(TableKey tk, ColKey::Idx col_idx)
StringInterner* DB::get_string_interner(TableKey tk, ColKey col_key)
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this? We are not expecting to reach this function, don't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I'll remove it in the target branch.

if (sd.data() == nullptr)
return 0;
if (sd.data() == nullptr) {
if (null_seen)
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is the machinery for handling a null string, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup.

@@ -340,7 +340,7 @@ void ArrayString::clear()
}
}

size_t ArrayString::find_first(StringData value, size_t begin, size_t end) const noexcept
size_t ArrayString::find_first(StringData value, size_t begin, size_t end, std::optional<StringID> id) const noexcept
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can pass the optional String ID by const ref? ... I guess it doesn't matter too much though, since StringID should be an integer right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should not make any difference.

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

2 participants