-
Notifications
You must be signed in to change notification settings - Fork 287
feat : Lookup table on multiple columns #1428
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 1 file
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="packages/tracecat-registry/tracecat_registry/core/table.py">
<violation number="1" location="packages/tracecat-registry/tracecat_registry/core/table.py:34">
lookup now returns a list instead of a single record or None, breaking the function’s return contract and callers’ expectations</violation>
<violation number="2" location="packages/tracecat-registry/tracecat_registry/core/table.py:66">
Typo in comment: "on value" should be "one value" for clarity</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai
to give feedback, ask questions, or re-run the review.
) | ||
# Since we set limit=1, we know there will be at most one row | ||
return rows[0] if rows else None | ||
return await lookup_many(table=table, column=column, value=value, limit=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lookup now returns a list instead of a single record or None, breaking the function’s return contract and callers’ expectations
Prompt for AI agents
Address the following comment on packages/tracecat-registry/tracecat_registry/core/table.py at line 34:
<comment>lookup now returns a list instead of a single record or None, breaking the function’s return contract and callers’ expectations</comment>
<file context>
@@ -23,23 +23,15 @@ async def lookup(
- )
- # Since we set limit=1, we know there will be at most one row
- return rows[0] if rows else None
+ return await lookup_many(table=table, column=column, value=value, limit=1)
</file context>
return await lookup_many(table=table, column=column, value=value, limit=1) | |
return next(iter(await lookup_many(table=table, column=column, value=value, limit=1)), None) |
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> Signed-off-by: y0no <y0no+github@y0no.fr>
Checklist
uv run pytest tests
)?pre-commit run --all-files
)?Description
This PR aims to provide a way to lookup on table over multiple columns and values. The implementation is compatible with the old syntax and the new one.
Currently the tests are not implemented in case the feature is not validated. I think there is a better way to implement this feature for example with a filter field which accept an array of key=value strings..But I don't know how to do it without breaking the old syntax.
Steps to QA
Summary by cubic
Add multi-column filtering to table lookups. You can now pass lists of columns and values; single-column calls still work.