[byKryptogram]

Our dependency github.com/parquet-go/parquet-go has now implemented support for shorter integers, use them to avoid having to define a manual schema and avoid the sorting drawback.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration

• Source mode: Auto
• Repository: dnstapir/edm
• Branch: mattias/session
• File: .coderabbit.yaml
• Commit: b326e1f
• Layers: Repository YAML
• Review profile: QUIET
• Plan: Advanced
• Run ID: de397349-7422-4c68-8c1c-8cbbf1a3610f

No actionable comments were generated in the recent review.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: dnstapir/edm/.coderabbit.yaml

Review profile: QUIET

Plan: Advanced

Run ID: 15a4ed40-99c5-4351-bbaf-95bd11a5a750

📥 Commits

Reviewing files that changed from the base of the PR and between 71b7001 and ed43677.

📒 Files selected for processing (5)

• pkg/runner/data_collector_test.go
• pkg/runner/manual_parquet_rotation_test.go
• pkg/runner/parquet_files_test.go
• pkg/runner/session.go
• pkg/runner/session_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

The sessionData struct now uses unsigned integer fields and byte slices instead of signed integers and string pointers. The hand-built Parquet schema is removed, and the writer uses the inferred schema. Tests are updated to the new field types.

Changes: Session data type migration

Layer / File(s)	Summary
sessionData types and inferred schema pkg/runner/session.go	sessionData uses uint fields and byte slices. newSession stores values directly and clones message bytes. sessionDataSchema is removed. writeSessionParquet uses the inferred schema.
Test fixtures for new field types pkg/runner/session_test.go, pkg/runner/data_collector_test.go, pkg/runner/manual_parquet_rotation_test.go, pkg/runner/parquet_files_test.go	Tests set ServerID as []byte. Session tests use unsigned pointers and create the writer without an explicit schema. Assertions compare ServerID as bytes.

Priority: Low

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Refactor

Suggested reviewers: linkdata

Merge Risk: Minimal · previous review ed436

The identified identity and schema concerns do not block merging; normal checks remain appropriate.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 5 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the main change: using the automatic schema for session Parquet files.
Description check	✅ Passed	The description explains that the change uses shorter integer support from parquet-go to remove the manual schema and its sorting drawback.

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[jschlyter]

ping @morkrost

[linkdata]

server_id, query_message and response_message changed from optional to required in the Parquet schema. A []byte field without the optional tag is written as required binary, so nil is stored as an empty value instead of NULL. For example, a query-only session now has response_message = '' where main wrote NULL.

Fix:

ServerID        []byte `parquet:"server_id,optional"`
QueryMessage    []byte `parquet:"query_message,optional"`
ResponseMessage []byte `parquet:"response_message,optional"`

A round-trip test that writes a query-only session and checks that response_message and server_id read back as NULL would catch this.

[linkdata]

LGTM

[byKryptogram]

server_id, query_message and response_message changed from optional to required in the Parquet schema. A []byte field without the optional tag is written as required binary, so nil is stored as an empty value instead of NULL. For example, a query-only session now has response_message = '' where main wrote NULL.

@linkdata: During my testing the struct fields could already result in a NULL value in the resulting parquet files since they are all nilable. However I've added the tag ,optional on all struct fields for good measure. Note that the fields in dnsLabels are now mandatory, instead of optional, but we can't change that without also changing the format of the histogram files(ie. histograms and parquet files behave in the same fashion now regarding dnsLabels).