-
-
Notifications
You must be signed in to change notification settings - Fork 283
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
Add postgresql schemata options #2028
Conversation
Add PgSchemataTable class Add support for options.postgres_schema "*" and "**": - open a schemata table - "*" show user schemata - "**" show user and system schemata - entering a schemata row open the table Sheet names follow the "database[.schema[.table]]" format
|
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.
Hi @jrjsmrtn, thanks for putting this together! This will be a useful addition. There are a few things that need to be adjusted, if you can make these changes, I think we're well on our way to getting it merged.
rowtype = 'schemas' | ||
|
||
def reload(self): | ||
if options.postgres_schema == "*": |
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.
Use self.options
(here at least, also other places it's used) so that the option can be overridden on a per-sheet basis.
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.
And let's make it so that an empty string means all-but-internal schema, instead of *
. Maybe *
can mean "including internal schemas" like you have for **
below. Do we think it's important to support internal schemas though?
qstr = """SELECT schema_name FROM information_schema.schemata;""" | ||
|
||
with self.sql.cur(qstr) as cur: | ||
self.nrowsPerTable = {} |
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.
Is this used anywhere?
return PgTable(self.name+"."+row[0], source=row[0], sql=self.sql) | ||
return PgTable( | ||
self.name+"."+row[0], | ||
source=(self.source or options.postgres_schema)+"."+row[0], |
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.
Seems like the given name and the source should be the same here. Let's use source
, since I think it's reasonable to require source
to be the schema for this class, and only use options.postgres_schema
in the outer openurl_postgres
function.
class PgSchemataSheet(Sheet): | ||
rowtype = 'schemas' | ||
|
||
def reload(self): |
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.
Generally we're standardizing on defining iterload()
, which is almost entirely like reload
, but you don't have to clear the rows, and instead of calling addRow()
directly, just yield
the row.
Ping @jrjsmrtn! |
Pong ! :-)Sorry for being MIA. I have not forgotten your comments. I hope to apply them next week-end.Le 15 nov. 2023 à 07:26, Anja Kefala ***@***.***> a écrit :
Ping @jrjsmrtn!
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
if self.options.postgres_schema: | ||
source = f"{self.options.postgres_schema}.{self.source}" | ||
else: | ||
source = self.source | ||
source = self.source |
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.
This will conflict with #2129, where the table name and schema were quoted to support both being something other than lowercase.
i.e.:
"public"."TableName"
rather than public.TableName
or "public.TableName"
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.
Thanks for checking! As part of this PR, should we then move the #2129 fix into openurl_postgres
?
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.
We will have to check that the loading of lowercase table names is preserved before merging.
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.
should we then move the #2129 fix into
openurl_postgres
?
I don't see how that could be done. I think my tweak belongs in these places, where PgTable.source
is set:
certainly here:
https://github.com/jrjsmrtn/visidata/blob/621dd88a75c6a133ce1a79cbd795c51fccd1d743/visidata/loaders/postgres.py#L167
maybe here?:
https://github.com/jrjsmrtn/visidata/blob/621dd88a75c6a133ce1a79cbd795c51fccd1d743/visidata/loaders/postgres.py#L123
I don't understand what some of the variables in this code refers to, such as row[0]
, columns[0:1]
, etc, so I'm on shaky ground 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.
row[0]
is the table_name
as per the rowdef
comment at the top of the class. columns[0:1]
creates a list containing the first column (and an empty list if there are no columns).
I don't think the quoting should be done in the schema.table
string given to the sheet; the SQL should do the quoting. So this means that the schema and the table name should be passed separately to the postgres sheet.
Hi @jrjsmrtn! We're going to close this for now due to lack of activity. Please re-open when you make the changes! |
Add PgSchemataTable class
Add support for options.postgres_schema "*" and "**":
Sheet names follow the "database[.schema[.table]]" format
Fixes #2027