L282 to L290 is still assuming that one and only one starting parameter will be set, are you intended to do so?

I prefer we do not make such assumption?

Also for starting_timestamp and ending_timestamp, we need to encode them for the server to be able to parse, something I found on web:

import urllib.parse
urllib.parse.quote(query)

Removed the assumption.

I tried your suggestion about urllib, but the server does not like it and it returns an error.

hmmm, I think we need to do sth like this, so that & is correctly added.

params_list = []
if cdfOptions.starting_version is not None:
  params_list.append(f"startingVersion={cdfOptions.starting_version}")
query_str = "&".join(params_list))

For the urllib, I think we need and only need to apply urllib.parse.quote on cdfOptions.starting_timestamp, so that it doesn't encode =, and it encodes the space .
This is due the the server behavior, verified in both Delta Sharing OSS server and our rpc parser: https://github.com/databricks/universe/pull/174415

And needs to add a corresponding test case to test the encoding behavior is correct in our client.

Done... good catch on the params!

Better to add a test case with starting_timestamp, to just ensure it's correctly encoded(so that the server is able to parse it).

But one thing to note is that, seems that when running tests locally the server timezone is PST, and when github runs tests the server timezone is GMT. So the same string startingTimestamp will be parsed as different timestamp (long), which will map to different version of the table, thus results in different testing behavior.

One trick I did is use a very large startingTimestamp and expect an error message: in #146 , DeltaSharingRestClientSuite:getCDFFiles with Timestamp,

Added a test for start timestamp.

As mentioned, needs to add a corresponding test case to test the encoding behavior is correct in our client, could you separate it out, and expect an error, similar to #146?

Though agree with you, we need to discuss whether it's safe to always expose DeltaStandaloneException, but in this case, we have to expose it because it's not internal error, and for the user/client to know that they need to set correct timestamp.

Done.
Also added a more specific exception mapping in the reader, which will be safer.

If you sync with the head, you don't need this change? The main concern is that history.getActiveCommitAtTime(ts) may throw other exceptions which may not be DeltaCDFIllegalArgumentException.

Or do you still prefer this change over what I did here? https://github.com/delta-io/delta-sharing/blob/main/server/src/main/scala/io/delta/sharing/server/DeltaSharingService.scala#L98

Neither is obviously better since both are not precisely catching and throwing the error.

The scope of getActiveCommitAt is a lot smaller, and looking at the code it will most commonly only throw timestamp related exception. So it is safer imho.

The other one seems too broad, so we should perhaps remove that one until we find a better solution.