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.

Sign up for GitHub

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

Jump to bottom

Fixing quesma common table dynamic mapping case #878

Closed
wants to merge 1 commit into from
Closed

Fixing quesma common table dynamic mapping case #878

wants to merge 1 commit into from

Conversation

pdelewski
Copy link
Contributor

@pdelewski pdelewski commented Oct 11, 2024 •
edited
Loading

This PR fixes the issue where quesma_common_table is used, and the mapping contains additional fields that were not present during ingestion.

processors:
  - name: my-query-processor
    type: quesma-v1-processor-query
    config:
      indexes:
        kibana_sample_data_ecommerce:
          target: [ my-clickhouse-data-source ]
          useCommonTable: true
          schemaOverrides:
            fields:
              "geoip.location":
                type: geo_point

Before :
image

After :
image

Additionally, it introduces:

  • The AlterDDL struct to represent ALTER statements in object form instead of as a string. My plan is to do more refactoring around this in subsequent PR(s).
  • A FieldOrigin property, which provides information about the source of the field's metadata.

@pdelewski pdelewski force-pushed the fixing-quesma-common-table-dynamic-mapping-case branch 3 times, most recently from e6d7800 to 87391ec Compare October 11, 2024 12:16
nablaone
nablaone reviewed Oct 11, 2024
View reviewed changes
quesma/ingest/processor.go
@@ -286,6 +286,13 @@ func getAttributesByArrayName(arrayName string,
return attributes
}

type AlterDDL struct {
tableName string
Copy link
Member

Choose a reason for hiding this comment

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

+1

@pdelewski pdelewski force-pushed the fixing-quesma-common-table-dynamic-mapping-case branch 2 times, most recently from 14c9be3 to 4045d55 Compare October 11, 2024 13:24
@pdelewski pdelewski force-pushed the fixing-quesma-common-table-dynamic-mapping-case branch from 4045d55 to 1a8a78b Compare October 11, 2024 13:25
@pdelewski pdelewski marked this pull request as ready for review October 11, 2024 13:33
@pdelewski pdelewski requested a review from a team as a code owner October 11, 2024 13:33
@pdelewski pdelewski mentioned this pull request Oct 11, 2024
Closed
@nablaone
Copy link
Member

What will happen?

  1. Sth puts a mappings (with field foo)
  2. Quesma restarts
  3. Sth starts ingesting (no foo field)

@pdelewski
Copy link
Contributor Author

What will happen?

  1. Sth puts a mappings (with field foo)
  2. Quesma restarts
  3. Sth starts ingesting (no foo field)

Right now, foo column created. That of course might cause a problem of creation columns that will be empty.

@pdelewski
Copy link
Contributor Author

I will try other idea to fix it

@pdelewski pdelewski mentioned this pull request Oct 14, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Oct 15, 2024
@pdelewski
Fixing quesma common table dynamic mapping case, second approach (#885)
8c93f77 
This is a second approach (1 -
#878) of fixing a problem where
mappings contains more fields than ingest and sql queries are based on
this knowledge. In this approach we are not altering table to have all
fields that were added by dynamic mapping but we based on field origin
information and building queries according to that.

<img width="1706" alt="image"
src="https://github.com/user-attachments/assets/40e52e56-7da5-4346-b138-8f5b738513c4">
@pdelewski
Copy link
Contributor Author

Closing as duplicate

@pdelewski pdelewski closed this Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nablaone nablaone Awaiting requested review from nablaone nablaone is a code owner automatically assigned from QuesmaOrg/engineering

@jakozaur jakozaur Awaiting requested review from jakozaur jakozaur is a code owner automatically assigned from QuesmaOrg/engineering

@mieciu mieciu Awaiting requested review from mieciu mieciu is a code owner automatically assigned from QuesmaOrg/engineering

@trzysiek trzysiek Awaiting requested review from trzysiek trzysiek is a code owner automatically assigned from QuesmaOrg/engineering

@avelanarius avelanarius Awaiting requested review from avelanarius avelanarius is a code owner automatically assigned from QuesmaOrg/engineering

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pdelewski @nablaone