billbelanger commented on PR #39538:
URL: https://github.com/apache/superset/pull/39538#issuecomment-5681225125
@Vitor-Avila
Thank you for doing this work, it has saved me a bunch of troubleshooting.
However, I'm curious if you might have some insight into a problem I ran into,
and my proposed solution. For background, I do not control the OpenSearch
system, I'm merely tapping into it with Superset, and I'm brand new to sqlglot
too. I don't really have a good setup to test any of this so I'm not sure of
the repercussions of the proposed change - other than "it works on my setup".
The issue I ran into is that the indexes in OpenSearch have dashes in their
names. Thus, they need to be quoted in superset (`"abc-def"` as opposed to
`abc-def`) or else it'll try to parse it as a subtraction. Both before and
after this patch, running the query through superset resulted in an error of
`404: IndexNotFoundException[no such index [\"abc-def\"]]`. Note that inside
the square brackets, the quotes were considered part of the table name.
OpenSearch only seems to accept backticks when using the SQL plugin, but
perhaps this is only for table names with dash characters; I'm not sure. This
was proven out using the `_plugins/_sql` API endpoint of the OpenSearch
instance.
```python
resp = requests.post("https://<hostname>/_plugins/_sql",
json={ "query": 'SELECT * FROM "abc-def" LIMIT 10' },
auth=HTTPBasicAuth("<username>", "<password>")
)
# Error
resp = requests.post("https://<hostname>/_plugins/_sql",
json={ "query": 'SELECT * FROM `abc-def` LIMIT 10' },
auth=HTTPBasicAuth("<username>", "<password>")
)
# Success
```
So, I changed the order of `IDENTIFIERS` to ``['`', '"']``. After this
change, my superset instance was able to query the OpenSearch setup
successfully. it also works if I ONLY have the backticks and remove the
double-quote entirely.
Again, I don't really have a setup to test this thoroughly, in fact the only
indexes I have access to ALL have dashes in their names. I'm sending this
message for two reasons: (1) I wouldn't want to create a PR if there's more
wide ranging issues with changing the `IDENTIFIERS` value, and (2) There's very
likely something I'm missing as to why the `"` character was included in the
first place if it doesn't seem to work. It seems like you would've run into
this issue based on the unit tests provided in this patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
