Fix failed UDF query#233
Conversation
chimosky
requested review from
a team,
EXBreder,
ayubun,
haileyok and
vinaysrao1
as code owners
Before now, using a UDF in a query threw this error;
File \"/osprey/osprey_worker/src/osprey/engine/query_language/ast_druid_translator.py\", line 102, in transform_Call
udf, _ = self._udf_node_mapping[id(node)]
~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
KeyError: 139836295173120"}
This happened because all the validators weren't being run during a
query, for some reason the REGISTRY didn't load all, just the additional
ones that are needed for a query.
DidAddLabel was also affected by this, this also fixes that.
I've also modified RegexMatch to use the right arguments as used in the
docs, item and regex created confusion as the docs stated target and
pattern, and using that in the query threw an error.
Signed-off-by: Chihurumnaya Ibiam <ibiamchihurumnaya@gmail.com>
chimosky
force-pushed
the
failed-udf-query
branch
from
b29e18f to
2c614aa
Compare
Signed-off-by: Chihurumnaya Ibiam <ibiamchihurumnaya@gmail.com>
|
oh yea, i encountered this before as well iirc. thanks for looking and fixing! can you pull out the uv.lock changes before we merge please? |
Signed-off-by: Chihurumnaya Ibiam <ibiamchihurumnaya@gmail.com>
Done, I know I wasn't supposed to include it but I'd run into some issues at the time with the pre-commit hook failing and I had to include it in the commit, forgot to revert it once I'd gotten past that. |
Signed-off-by: Chihurumnaya Ibiam <ibiamchihurumnaya@gmail.com>
|
yea no worries. tbh we might be able to come up with some sort of hook that checks to make sure that doesn't happen, but im not really sure what the best wya to go about it is if im being honest |
|
|
||
| def test_regex_match_accepts_valid_call(run_validation: RunValidationFunction) -> None: | ||
| run_validation("RegexMatch(item=A, regex='^foo$')") | ||
| run_validation("RegexMatch(target=A, pattern='^foo$')") |
There was a problem hiding this comment.
i believe that test snapshots need to get updated for this (and others)
There was a problem hiding this comment.
| def _consolidate_registry(self, validator_registry: ValidatorRegistry) -> ValidatorRegistry: | ||
| validators = { | ||
| validator | ||
| for validator in ValidatorRegistry.get_instance().get_validators() | ||
| if not validator.exclude_from_query_validation | ||
| } | ||
| registry_validators = set() | ||
| for validator in validator_registry.get_validators(): | ||
| registry_validators.add(validator) | ||
|
|
||
| return ValidatorRegistry.from_validator_classes(validators.union(registry_validators)) |
There was a problem hiding this comment.
does doing this break any existing tests or functionality? it seems like this may break cases where we try to explicitly use only one or two validators (i think mostly for testing).
There was a problem hiding this comment.
Yes, it does break existing functionality and I've fixed that.
How do I update test snapshots as it seems that's the only reason the test is failing right now, the test says to use --write-outputs option and I can see the argument should be loaded as part of the engine.conftest plugin but it seems that happens later than it should so it's basically nonexistent.
I can't run pytest --snapshot-update in the directory as I don't have osprey and gevent installed locally, as they're supposed to be installed in the dev environment.
Might give it a look and see if I figure something out. |
I've moved loading all the registeries needed for a query to when a query is made, this is where it should be and it shouldn't affect existing behaviour. Signed-off-by: Chihurumnaya Ibiam <ibiamchihurumnaya@gmail.com>
chimosky
force-pushed
the
failed-udf-query
branch
from
95cec18 to
6d21e09
Compare
3 participants
Description
Before now, using a UDF in a query threw this error;
This happened because all the validators weren't being run during a query, for some reason the REGISTRY didn't load all, just the additional ones that are needed for a query.
DidAddLabel was also affected by this, this also fixes that.
Fixes #158
I've added changes to
uv.lockbecause the precommit hook fails without it for some reason;Checklist
uv run ruff check .passes (no unused imports or other lint errors)uv tool run fawltydeps --check-unused --pyenv .venvpasses (no unused dependencies)CHANGELOG.mdwith my changes, if applicable