Support sheet table function for google sheets connector

[grantatspothero]

Description

Supports sheet table function for the google sheets connector.

Allows querying google sheets directly by id instead of requiring a metadata sheet mapping.

Also makes the gsheets.metadata-sheet-id config optional since the sheet table function makes it no longer strictly required.

Additional context and related issues

N/A

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Google Sheets
* Add `sheet` table function to the connector. ({issue}`12502`)

[grantatspothero]

Ready for another review.

[ebyhr]

Could you confirm and fix the commit boundaries? check-commit failed due to a missing symbol.

[grantatspothero]

Could you confirm and fix the commit boundaries? check-commit failed due to a missing symbol.

Removed toSchemaTableName in one commit too early, fixed.

[anusudarsan]

@kasiafi or @martint can you please do a final review/approve (and merge) this?

[grantatspothero]

@wendigo mind giving a final set of eyes? @ebyhr already gave a detailed review.

[findepi]

Engine is responsible for serializing the handles, but i don't think it cares about their common interfaces.
Thus i think we can separate SheetsConnectorTableHandle serialization and ignore (not test) whether jsonCodec(SheetsConnectorTableHandle) is able to find the subclasses.

[grantatspothero]

I just confirmed, you are correct this bit is not necessary.

Will remove and just test that the specific table handles can be serialized/deserialized.

[findepi]

cast of expected is redundant. why not just declare the variable with a more specific type

[grantatspothero]

Removed after the above refactor.

[findepi]

how does googleSheetIdHere typically look like? are they numeric, alphanumeric?

[grantatspothero]

See TestSheetsPlugin for an example sheet id. For example: 1S625j2oTptRepg6Yci68fCYE1269tdoSjljNOmTgQ3U

[findepi]

Looks verbose. @mosabua @m57lyra is it how we typically format SQL in docs?

[mosabua]

Following the Mozilla SQL style ideally .. in this case I would confirmhow we format in all other docs .. but I think this is right

[grantatspothero]

I copied this formatting from the other table function docs, example: https://github.com/trinodb/trino/blob/master/docs/src/main/sphinx/connector/mongodb.rst#table-functions

[findepi]

is this formatting compatible with how Trino SQL formatter would format this?

[grantatspothero]

Did this in a debugger:

var statement = new SqlParser().createStatement("SELECT * FROM TABLE(example.system.sheet(id => 'googleSheetIdHere', range => 'TabName!A1:B4'))", new ParsingOptions());

SqlFormatter.formatSql(statement);

The formatter decides to write the query like this:

SELECT *
FROM
  TABLE(example.system.sheet(
      id => 'googleSheetIdHere',
      range => 'TabName!A1:B4'))

I did not know that formatter existed, good to know. Changed.

[findepi]

How long does it take to run this test class?

[grantatspothero]

pretty fast, ~15 seconds on my local machine. most of which is creating the catalog.

[findepi]

what about adding a test case in TestSheetsPlugin (instead?)

[grantatspothero]

I do not think that helps, TestSheetsPlugin runs in 1-2 seconds currently.

Moving the test case in there will just make TestSheetsPlugin take ~15 seconds instead of ~1-2 seconds. I need to not just verify the connector can startup without the metadata sheet id, but also that the connector can run a table function query successfully without a metadata id.

[findepi]

@grantatspothero i understand the approach currently taken is best from test coverage perspective.
@nineinchnick and I were recently debating our inherently limited ability to have test coverage, and how much we can test in total on the CI. Looks like there are and will always be limits to that. This made me more cautious about time being added to tests with respect to additional confidence gained when doing so. Ultimately, we need to accept the fact we cannot test everything, which is especially sad to me.

(no change requested here, but I'd still consider not having this test class)

[mosabua]

Need to mention that this is the example catalog..

[grantatspothero]

fixed.

[findepi]

what 2 and 5 are referring to?

[grantatspothero]

The error codes for: SHEETS_UNKNOWN_TABLE_ERROR and NOT_FOUND respectively. changed the comment to use the names instead of the codes to be clearer.

[findepi]

LGTM

thanks @grantatspothero for quick response loop

[findepi]

thanks, now i can read the comment at least :)

however, i don't understand why we would want to throw "not found" within IOException catch block
to me, the current SHEETS_UNKNOWN_TABLE_ERROR looks fine (the IOException is just some error)

if we want to detect that IOException conveys "specific sheet doesn't exist", we could do that (but we wouldnt' say TableNotFound even then, because the requested sheet may not be a table)

if it was up to me to decide, and if no new information is provided to me, i would just delete this TODO comment and worry about that later, and only if we learn later there is something to worry about

[findepi]

(rebased to resolve conflict)

[findepi]

/test-with-secrets sha=d3ed6c25f1b3add9cdd97d9590780e24ecc3a38d

[findepi]

CI green. Waiting for secrets run to finish.

[grantatspothero]

Could not find the secret run here: https://github.com/trinodb/trino/actions?query=event%3Arepository_dispatch

I see other secrets runs from other PRs, but not for the commit d3ed6c25f1b3add9cdd97d9590780e24ecc3a38d

[findepi]

@grantatspothero thanks for reminding me we don't need a run with secrets for this PR