-
Notifications
You must be signed in to change notification settings - Fork 1
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
Support virtual SQL for JSON TABLE #4
Conversation
Signed-off-by: shanhaikang.shk <[email protected]>
Signed-off-by: shanhaikang.shk <[email protected]>
Signed-off-by: shanhaikang.shk <[email protected]>
Signed-off-by: shanhaikang.shk <[email protected]>
Signed-off-by: shanhaikang.shk <[email protected]>
Signed-off-by: shanhaikang.shk <[email protected]>
Session = sessionmaker(bind=self.engine) | ||
session = Session() | ||
new_meta_cache_items = [] | ||
col_id = 16 |
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.
why does col_id
starts with 16?
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.
The OceanBase
user column ID also starts with 16, which can be considered an easter egg.
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.
There doesn't seem to be any problem, but it also doesn't feel very necessary.
**kwargs, | ||
): | ||
super().__init__(uri, user, password, db_name, **kwargs) | ||
self.Base.metadata.create_all(self.engine) |
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.
Will this DDL
work as expectation?
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.
DBA will create these two tables in advance so that even users without DDL permissions can use them normally.
def refresh_metadata(self): | ||
self.jmetadata.reflect(self.engine) | ||
|
||
def perform_json_table_sql(self, sql: str): |
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.
Recommend to add type annotations for function return value
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.
done
if jtable_name in self.jmetadata.meta_cache: | ||
raise ValueError("Table name duplicated") | ||
|
||
Session = sessionmaker(bind=self.engine) |
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.
I think it would be better to declare the sessionmaker as an attribute of the client class.
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.
done
try: | ||
session.commit() | ||
self.jmetadata.meta_cache[jtable_name] = new_meta_cache_items | ||
logger.info(f"ADD METADATA CACHE ---- {jtable_name}: {new_meta_cache_items}") |
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.
Maybe logger.debug
is more appropriate here.
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.
done
Signed-off-by: shanhaikang.shk <[email protected]>
Signed-off-by: shanhaikang.shk <[email protected]>
Signed-off-by: shanhaikang.shk <[email protected]>
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.
LGTM
else: | ||
ast.args['joins'] = [join_node] | ||
|
||
extra_filter_str = f"user_id = {self.user_id} AND jtable_name = '{table_name}'" |
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.
Add a table name reference to avoid conflicts; for example, maybe user_id can exist in user-defined table
Session = sessionmaker(bind=self.engine) | ||
session = Session() | ||
new_meta_cache_items = [] | ||
col_id = 16 |
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.
There doesn't seem to be any problem, but it also doesn't feel very necessary.
Summary
Solution Description
introduce
sqlglot
&pydantic
into pyobvector