Skip to content

Conversation

wraymo
Copy link
Member

@wraymo wraymo commented May 15, 2024

References

#403

Description

Previously, when a query did not contain a subquery, it was not evaluated in clp-s. However, in cases where matchability = SubQueryMatchabilityResult::SupercedesAllSubQueries, this caused an issue. This PR fixes the bug, which indirectly resolves issue #403. The primary code change aligns clp-s with the behavior of clp in Grep::process_raw_query. The relevant code in clp_s/search/Output.cpp has also been updated.

Validation performed

Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

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

Nice work. A few minor comments.

#include "../ArchiveReader.hpp"
#include "../FileWriter.hpp"
#include "../ReaderUtils.hpp"
#include "../Utils.hpp"
Copy link
Member

Choose a reason for hiding this comment

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

I think StringUtils still needs this as a direct include.

// don't't add more wildcards. Likewise if it already contains some wildcards
// we do not add more
Grep::process_raw_query(
m_string_query_map[query_string] = std::move(Grep::process_raw_query(
Copy link
Member

Choose a reason for hiding this comment

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

Shall we switch to emplace instead?

@wraymo wraymo requested a review from kirkrodrigues June 2, 2024 23:34
kirkrodrigues
kirkrodrigues previously approved these changes Jun 3, 2024
Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

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

One small suggestion.

For the PR title, how about:

clp-s: Handle cases where clp-string query-parsing falls back to decompress + match (fixes #403).

bool is_var;
// FIXME: may want to use non-heuristic method of tokenizing query
// if (use_heuristic) {

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

@wraymo wraymo requested a review from kirkrodrigues June 3, 2024 15:15
@wraymo wraymo changed the title clp-s: Correctly handle SubQueryMatchabilityResult::SupercedesAllSubQueries case (fixes #403). clp-s: Handle cases where clp-string query-parsing falls back to decompress + match (fixes #403). Jun 3, 2024
@wraymo wraymo merged commit 056ee6c into y-scope:main Jun 3, 2024
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