Skip to content

Conversation

divinerapier
Copy link

Correct inaccuracies in the documentation regarding session_properties configuration.

const mapKeySeparator   = ":"
const mapEntrySeparator = ";"

func decodeMapHeader(name, input string) ([]string, error) {
	result := []string{}
	for _, entry := range strings.Split(input, mapEntrySeparator) {
		parts := strings.SplitN(entry, mapKeySeparator, 2)
	}
	return result, nil
}

Copy link

cla-bot bot commented May 29, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Copy link
Member

@nineinchnick nineinchnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this. I introduced this breaking change in #127

Copy link

cla-bot bot commented May 30, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@nineinchnick nineinchnick added the documentation Improvements or additions to documentation label May 30, 2025
@nineinchnick nineinchnick changed the title fix(doc): use colon instead of equals sign in session properties Document using colon instead of equals sign in session properties May 30, 2025
@Flgado
Copy link
Member

Flgado commented May 30, 2025

Hey team @divinerapier @nineinchnick , just a quick note:
With this solution, if the ; characters is not URL-encoded on the dns, nothing will be passed to the X-Trino-Session header. This is because url.Query() (here) does not treat session_properties as valid in that case and ignore it. The library silently discards malformed value pairs but I detect that when I was working on X-Trino-Role header and it does not accept ;

@divinerapier
Copy link
Author

Hi @Flgado

Thanks for raising this important point about URL encoding and silent failures—it's a great catch! 😊

I agree this potential edge case deserves attention. However, since ​​this PR primarily focuses on fixing documentation consistency​​ (aligning session_properties syntax with Trino's actual behavior), I propose we handle it in two steps:

​​Merge the current doc fix​​ to avoid user confusion. ✅

​​Open a dedicated issue​​ to deeply investigate:
The scope of unencoded ; in session properties,
How other Trino SDKs (e.g., Python/Java) handle this,
Whether to enforce encoding or add validation/errors.
This way, we ensure incremental progress while giving the encoding problem the thorough exploration it needs. Would you be open to this approach? I’m happy to collaborate on the follow-up issue! 🙌

@Flgado
Copy link
Member

Flgado commented Jun 1, 2025

Hi @divinerapier

Totally agree! Just wanted to share this with you both because I was also caught by surprise.

Let's open an issue—I'm happy to pair up and help fix this 😃

Thanks!

Copy link

cla-bot bot commented Jun 5, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@divinerapier
Copy link
Author

Hi team, especially @nineinchnick and @Flgado 👋

Quick CLA update:​​ I've submitted my signed CLA to [email protected] from ​​[email protected]. Could someone please help verify when possible? 🙇

I've pushed an important fix addressing the ​​intermittent unit test failures​​ we observed

(integration_test.go:func waitForTrinoReady).

​​Root cause identified:​​
🔍 Tests were failing because queries executed ​​before TrinoDB fully initialized​​, causing queries to not be assigned to compute nodes.

​​Fix implemented:​​
✅ Added explicit waiting logic to ensure TrinoDB is ​​fully operational​​ before test execution
✅ Implemented health check validation for cluster readiness
✅ Maintained all previous documentation fixes

This resolves the flaky tests while preserving the original session_properties documentation correction. Would you mind taking another look when convenient?

@divinerapier divinerapier requested a review from nineinchnick June 5, 2025 02:25
@@ -474,6 +491,7 @@ func integrationOpen(t *testing.T, dsn ...string) *sql.DB {
if err != nil {
t.Fatal(err)
}
waitForTrinoReady(t, db)
Copy link
Member

@nineinchnick nineinchnick Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method should not be receiving t, and logging of any errors should happen here.

Also, it because it queries the system catalog, it makes some tests redundant, like TestIntegrationSelectQueryIterator. Would executing a SELECT 1 query be sufficient? If yes, can we execute this query using the trino command inside the container, after https://github.com/trinodb/trino-go-client/blob/master/trino/integration_test.go ? This way it'll be independent of the correctness of this driver.

In any case, this should be done in a separate commit.

How does the failure manifest itself? We use a single node instance in these tests.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the recent changes to the integration test logic:

​​Reason for modification​​
I encountered intermittent No nodes available to run query errors during local test execution. While we check container health via waitForContainerHealth, there appears to be a gap between container readiness and actual service availability. My changes address this by adding explicit service-level verification before executing queries.

CI pipeline issue​​
For the GitHub CI failure observed here, I'm currently investigating but would appreciate your insights.

@nineinchnick
Copy link
Member

@cla-bot check

Copy link

cla-bot bot commented Jul 13, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Copy link

cla-bot bot commented Jul 13, 2025

The cla-bot has been summoned, and re-checked this pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Development

Successfully merging this pull request may close these issues.

3 participants