Skip to content

clp-package: Fix bugs introduced in #460:#481

Merged
haiqi96 merged 1 commit into
y-scope:mainfrom
haiqi96:bug_fixes
Jul 15, 2024
Merged

clp-package: Fix bugs introduced in #460:#481
haiqi96 merged 1 commit into
y-scope:mainfrom
haiqi96:bug_fixes

Conversation

@haiqi96

@haiqi96 haiqi96 commented Jul 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

This PR fixes two bugs in query worker scripts, introduced by #460

  1. Fixes a bug where the search results are written to task_id collection. They should be written to job_id collection
  2. Fixes a bug where an int is converted to string before joined as part of the command

Validation performed

Tested with log viewer.

@kirkrodrigues kirkrodrigues requested review from kirkrodrigues and removed request for kirkrodrigues July 15, 2024 15:02
junhaoliao
junhaoliao approved these changes Jul 15, 2024
View reviewed changes

@junhaoliao junhaoliao left a comment

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.

Lgtm. How about changing the PR title to "clp-package: Fix type conversions in query worker scripts."?

@haiqi96

haiqi96 commented Jul 15, 2024

Copy link
Copy Markdown
Contributor Author

Lgtm. How about changing the PR title to "clp-package: Fix type conversions in query worker scripts."?

hmm, it's not only type conversion. the change also updated the result collection to use job_id instead of task_id.

@junhaoliao

junhaoliao commented Jul 15, 2024

Copy link
Copy Markdown
Member

Lgtm. How about changing the PR title to "clp-package: Fix type conversions in query worker scripts."?

hmm, it's not only type conversion. the change also updated the result collection to use job_id instead of task_id.

Right, sorry for the oversight. How about: clp-package: Fix issues with command generation and search results write destination in query worker scripts.

@haiqi96

haiqi96 commented Jul 15, 2024

Copy link
Copy Markdown
Contributor Author

Quote reply
Reference in new issue
Edit
Hide
Delete

Lgtm. How about changing the PR title to "clp-package: Fix type conversions in query worker scripts."?

hmm, it's not only type conversion. the change also updated the result collection to use job_id instead of task_id.

Right, sorry for the oversight. How about: clp-package: Fix issues with command generation and search results write destination in query worker scripts.

@kirkrodrigues any suggestions?

@kirkrodrigues

kirkrodrigues commented Jul 15, 2024

Copy link
Copy Markdown
Member

How about:

clp-package: Fix bugs introduced in #460:

  • Write search results to collection named job_id rather than task_id.
  • Convert int to str in IR extraction command generation.

Ordinarily, I'd make these two separate PRs so the commit message is easier to write, but above is how I'd write it so that it makes sense why we're fixing both bugs in one PR.

@haiqi96 haiqi96 changed the title clp-package: Fix several bugs in query worker scripts clp-package: Fix bugs introduced in #460: Jul 15, 2024
@haiqi96 haiqi96 merged commit 862fccf into y-scope:main Jul 15, 2024
jackluo923 pushed a commit to jackluo923/clp that referenced this pull request Dec 4, 2024
@haiqi96 @jackluo923
clp-package: Fix bugs introduced in y-scope#460: (y-scope#481)
ad41ef5 
- Write search results to collection named job_id rather than task_id.
- Convert int to str in IR extraction command generation.
@haiqi96 haiqi96 deleted the bug_fixes branch December 6, 2024 20:32
junhaoliao pushed a commit to junhaoliao/clp that referenced this pull request May 17, 2026
@haiqi96
clp-package: Fix bugs introduced in y-scope#460: (y-scope#481)
bb4d875 
- Write search results to collection named job_id rather than task_id.
- Convert int to str in IR extraction command generation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@junhaoliao junhaoliao junhaoliao approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@haiqi96 @junhaoliao @kirkrodrigues