chore: reducing coverage loss #619
Conversation
| rbatch | ||
| ), f"Left and Right batch does not have equal elements: left: {len(lbatch)} right: {len(rbatch)}" | ||
|
|
||
| assert self.etype in [ |
There was a problem hiding this comment.
Won't keeping it in the else clause easier to maintain? Similar to the switch clause in cpp. Future changes won't need to worry about modifying assert. What do you think?
There was a problem hiding this comment.
Good idea. But, it will be harder to hit the else without a synthetic test case (from a coverage standpoint).
There was a problem hiding this comment.
But, if the assert is not updated and a new expression type is added, then they will have to update the assert before passing the test case. So, they will be forced to update the assert
There was a problem hiding this comment.
How about we move the assert condition to the test case? Any update to the logic will fail the test case and will require updating it.
It is a one-time effort, either we do it in the code using assert or in the test. I prefer test from readability perspective. What do you think?
There was a problem hiding this comment.
I am not entirely sure if I understand. Can you take care of this change?
There was a problem hiding this comment.
Like eva/catalog/catalog_manager.py, shall we also replace logger.execption and raise BinderError with assert for consistency?
eva/executor/delete_executor.py
Outdated
| except Exception as e: | ||
| logger.error(e) | ||
| raise ExecutorError(e) | ||
| if table_catalog.table_type == TableType.STRUCTURED_DATA: |
There was a problem hiding this comment.
We do not need this if check, the previous assert ensures that table_catalog.table_type is TableType.STRUCTURED_DATA
| raise ExecutorError(e) | ||
|
|
||
| def __call__(self, *args, **kwargs) -> Generator[Batch, None, None]: | ||
| yield from self.exec(*args, **kwargs) |
There was a problem hiding this comment.
Is ray supporting lateral join now? I think the __call__ is needed for ray.
There was a problem hiding this comment.
Not sure about this \ cc @kaushikravichandran @jiashenC @gaurav274
There was a problem hiding this comment.
Even if it were the case, this code was not being used in any of the test cases and repeated in a few operators.
There was a problem hiding this comment.
Ideally, I think it could be triggered by Ray if there is a LateralJoin under Projection. Maybe we can move this to the AbstractExecutor to improve the coverage? I think it will anyway have the same function implementation for any executor.
| raise ExecutorError( | ||
| f"StorageEngine {storage_engine} create call failed" | ||
| ) | ||
| storage_engine.create(table_obj) |
There was a problem hiding this comment.
Here do we still need to check the success from create? We probably don't want the next write if create failed.
There was a problem hiding this comment.
Changing this logic breaks these two test cases -- test_should_fail_to_load_videos_with_same_path and test_should_fail_to_load_images_with_same_path
storage_engine = StorageEngine.factory(table_obj)
if do_create:
create_status = storage_engine.create(table_obj)
if create_status:
storage_engine.write(
table_obj,
Batch(pd.DataFrame({"file_path": valid_files})),
)
There was a problem hiding this comment.
It is fine. create will raise error
| setattr(result, k, deepcopy(v, memo)) | ||
| return result | ||
|
|
||
| def copy(self): |
There was a problem hiding this comment.
Is copy not needed anymore?
eva/models/storage/batch.py
Outdated
| for column in by: | ||
| if column not in self._frames.columns: | ||
| raise KeyError( | ||
| "Can not orderby non-projected column: {}".format(column) |
There was a problem hiding this comment.
Can we also remove this raise?
| raise BinderError("Index input needs to be float32.") | ||
| if not len(output.array_dimensions) == 2: | ||
| raise BinderError("Index input needs to be 2 dimensional.") | ||
| assert IndexType.is_faiss_index_type( |
There was a problem hiding this comment.
Just curious, if we support index type other than Faiss later, we still need to do it with if else right (assuming we have some index specific checks)?
|
This PR affects |
Moving to a different pattern for handling exceptions (at the query executor instead of doing it in each operator) -- https://www.youtube.com/watch?v=8kTlzR4HhWo&list=PLqapmkczup-VyUEZW8adgVWX088cToWlX&index=4
Ref: https://www.reddit.com/r/learnpython/comments/yxcqib/exception_handling_best_practices/
Abstract tests:
https://github.com/georgia-tech-db/eva/blob/1d826440368682ce589735dba3cb87365d98d2b2/test/udfs/test_abstract_udf.py#L26-L40