Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Array support to Map #158

Merged
merged 2 commits into from
Oct 23, 2024
Merged

Add Array support to Map #158

merged 2 commits into from
Oct 23, 2024

Conversation

julienbourdeau
Copy link
Contributor

The latest version added support for Map (Thank you @danielwestendorf!)

At Lago, we forked this gem to add support for map but never took the time to open a PR with our change (sorry!). I'm updating our code to use the gem as much as possible. Our implementation was slightly different but very close.

The big missing feature is support for arrays inside maps like { key: [1, 2, 3] }, which I'm adding in this PR.

We'll be running it in production this week and we'll let you know how it goes.

What do you think @danielwestendorf ? Did you get to work with maps of arrays.

@danielwestendorf
Copy link
Contributor

Non-primitive types in maps isn’t something we’ve played with.

This implementation does feel like an awkward API in the ActiveRecord sense, however, I don’t have a great alternative to suggest. I wonder if any other AR adapters have tackled this problem and if we can take inspiration from their api implementation?

@julienbourdeau
Copy link
Contributor Author

Thank you for your feedback @danielwestendorf. If you're able to point what makes you feel awkward about the API, I'm happy to try to improve it 🙏

@PNixx PNixx merged commit a59294e into PNixx:master Oct 23, 2024
16 checks passed
vincent-pochet added a commit to getlago/lago-api that referenced this pull request Oct 29, 2024
## Context

Two pull requests opened on the `clickhouse-active` records repository
were merged lately.
- PNixx/clickhouse-activerecord#158
- PNixx/clickhouse-activerecord#169

Since now all mandatory pieces that we require to use it in production
are here, we can use the official gem rather than the fork.

## Description

This PR replaces `gem 'clickhouse-activerecord', git:
'https://github.com/getlago/clickhouse-activerecord.git'` with `gem
"clickhouse-activerecord", "~> 1.2.0"`
@julienbourdeau julienbourdeau deleted the map-add-array-support branch November 29, 2024 06:06
dmitrytrager added a commit to mayak-bz/clickhouse-activerecord that referenced this pull request Jan 20, 2025
* new-master: (77 commits)
  Use a flag to track updates/deletes instead of guessing (PNixx#188)
  Do not squish table definitions in schema dump. (PNixx#187)
  Add support for integer limits in map type (PNixx#178)
  Improve function dumps (PNixx#179)
  Reliably sort functions, views, and materialized views in schema (PNixx#181)
  Maintain primary key type specificity (PNixx#183)
  Support create table with request settings
  Support codec column PNixx#135, refactoring
  Add Array support to Map (PNixx#158)
  SchemaDumper adds materialized view destination (PNixx#159)
  Improve DB::Exception error handling (PNixx#165)
  Add blank line after create_function in schema (PNixx#170)
  Include column definitions in schema dump if the column name is not `id` (PNixx#173)
  Add support for LIMIT BY clause (PNixx#169)
  Add support for force `CREATE FUNCTION` (PNixx#146)
  Implement GROUP BY GROUPING SETS with new `group_by_grouping_sets` query method. (PNixx#161)
  fix for function creation in structure.sql (PNixx#166)
  fix schema dumper
  fix spec
  fix detect model primary key for older clickhouse versions
  ...
dmitrytrager added a commit to mayak-bz/clickhouse-activerecord that referenced this pull request Jan 20, 2025
* new-master: (77 commits)
  Use a flag to track updates/deletes instead of guessing (PNixx#188)
  Do not squish table definitions in schema dump. (PNixx#187)
  Add support for integer limits in map type (PNixx#178)
  Improve function dumps (PNixx#179)
  Reliably sort functions, views, and materialized views in schema (PNixx#181)
  Maintain primary key type specificity (PNixx#183)
  Support create table with request settings
  Support codec column PNixx#135, refactoring
  Add Array support to Map (PNixx#158)
  SchemaDumper adds materialized view destination (PNixx#159)
  Improve DB::Exception error handling (PNixx#165)
  Add blank line after create_function in schema (PNixx#170)
  Include column definitions in schema dump if the column name is not `id` (PNixx#173)
  Add support for LIMIT BY clause (PNixx#169)
  Add support for force `CREATE FUNCTION` (PNixx#146)
  Implement GROUP BY GROUPING SETS with new `group_by_grouping_sets` query method. (PNixx#161)
  fix for function creation in structure.sql (PNixx#166)
  fix schema dumper
  fix spec
  fix detect model primary key for older clickhouse versions
  ...
dmitrytrager added a commit to mayak-bz/clickhouse-activerecord that referenced this pull request Jan 21, 2025
* new-master: (77 commits)
  Use a flag to track updates/deletes instead of guessing (PNixx#188)
  Do not squish table definitions in schema dump. (PNixx#187)
  Add support for integer limits in map type (PNixx#178)
  Improve function dumps (PNixx#179)
  Reliably sort functions, views, and materialized views in schema (PNixx#181)
  Maintain primary key type specificity (PNixx#183)
  Support create table with request settings
  Support codec column PNixx#135, refactoring
  Add Array support to Map (PNixx#158)
  SchemaDumper adds materialized view destination (PNixx#159)
  Improve DB::Exception error handling (PNixx#165)
  Add blank line after create_function in schema (PNixx#170)
  Include column definitions in schema dump if the column name is not `id` (PNixx#173)
  Add support for LIMIT BY clause (PNixx#169)
  Add support for force `CREATE FUNCTION` (PNixx#146)
  Implement GROUP BY GROUPING SETS with new `group_by_grouping_sets` query method. (PNixx#161)
  fix for function creation in structure.sql (PNixx#166)
  fix schema dumper
  fix spec
  fix detect model primary key for older clickhouse versions
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants