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

sql: Fix incorrect handling of types #41607

Merged
merged 5 commits into from
Nov 19, 2024
Merged

sql: Fix incorrect handling of types #41607

merged 5 commits into from
Nov 19, 2024

Conversation

shmsr
Copy link
Member

@shmsr shmsr commented Nov 12, 2024

Proposed commit message

This fixes the incorrect handling of types in SQL module. More details on the issue are present here: #40090

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • Test with Elasticsearch
  • Test if valid inputs are treated correctly
  • Test if there are other cases too

How to test this PR locally

As usual.

Related issues

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Nov 12, 2024
@shmsr shmsr self-assigned this Nov 12, 2024
Copy link
Contributor

mergify bot commented Nov 12, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @shmsr? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit

Copy link
Contributor

mergify bot commented Nov 12, 2024

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

@mergify mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Nov 12, 2024
@shmsr shmsr added Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team bugfix labels Nov 12, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Nov 12, 2024
@@ -345,19 +345,28 @@ func inferTypeFromMetrics(ms mapstr.M) mapstr.M {

for k, v := range ms {
switch v.(type) {
case float64:
case int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64, float32, float64:
Copy link
Member Author

Choose a reason for hiding this comment

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

This is where based on types, key-vals are assigned under numeric, string or bool

@shmsr
Copy link
Member Author

shmsr commented Nov 13, 2024

Sample output:

Before this patch:

 "sql": {
        "query": "select '0054321'::varchar(20) as strCol, 12345 as intCol",
        "metrics": {
            "numeric": {
                "strcol": 54321,
                "intcol": 12345
            }
        },
        "driver": "postgres"
    }

After this patch:

"sql": {
        "driver": "postgres",
        "query": "select '0054321'::varchar(20) as strCol, 12345 as intCol",
        "metrics": {
            "string": {
                "strcol": "0054321"
            },
            "numeric": {
                "intcol": 12345
            }
        }
    }

So this is behaviour change in how data is now stored in ES but it's the correct change that's fixing a bug. We have to account this as well. But the thing is VARCHAR fields is now coming correctly under "sql.metrics.string" object as it should.

@shmsr shmsr marked this pull request as ready for review November 14, 2024 17:26
@shmsr shmsr requested review from a team as code owners November 14, 2024 17:26
@shmsr shmsr requested review from AndersonQ and faec November 14, 2024 17:26
@shmsr
Copy link
Member Author

shmsr commented Nov 18, 2024

@AndersonQ / @faec Could you please review this PR? This is an important bugfix that requires immediate attention. Thank you for your help.

We are planning to get this merged ASAP to make it available for next release.

@@ -167,24 +167,25 @@ func ReplaceUnderscores(ms mapstr.M) mapstr.M {
}

func getValue(pval *interface{}) interface{} {
Copy link
Contributor

@devamanv devamanv Nov 18, 2024

Choose a reason for hiding this comment

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

One suggestion(not related to your changes though):
We seldom need a pointer to an interface, unless there's a specific use case for it. It's like using a pointer to a pointer to an underlying concrete type. Also, we could use the any alias, which denotes the blank interface type interface{}. Just a suggestion, see if we can try to improve this a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I agree with your point. There are many code smells that need to be addressed. I'll create a task and add it to our backlog.

@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Nov 18, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

Copy link
Member

@cmacknz cmacknz left a comment

Choose a reason for hiding this comment

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

I have only skimmed this, approving to unblock.

Probably you could adjust the CODEOWNER so the data plane team isn't ping for this change next time.

@shmsr shmsr enabled auto-merge (squash) November 19, 2024 07:00
@shmsr shmsr merged commit b219763 into elastic:main Nov 19, 2024
30 of 32 checks passed
mergify bot pushed a commit that referenced this pull request Nov 19, 2024
shmsr added a commit that referenced this pull request Nov 19, 2024
* sql: Fix incorrect handling of types (#41607)

(cherry picked from commit b219763)

* Update CHANGELOG.next.asciidoc

* Update CHANGELOG.next.asciidoc

---------

Co-authored-by: subham sarkar <[email protected]>
@shmsr shmsr added the backport-8.16 Automated backport with mergify label Nov 21, 2024
mergify bot pushed a commit that referenced this pull request Nov 21, 2024
shmsr added a commit that referenced this pull request Nov 21, 2024
* sql: Fix incorrect handling of types (#41607)

(cherry picked from commit b219763)

* Update CHANGELOG.next.asciidoc

---------

Co-authored-by: subham sarkar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.x Automated backport to the 8.x branch with mergify backport-8.16 Automated backport with mergify bugfix Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants