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

KXI-58865 - [structuredText] char result renders incorrectly #499

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

Philip-Carneiro-KX
Copy link
Collaborator

Changes introduced by this PR

  • fix char type wrapper bug for local q

quantity
];
quantity
]; columns)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need this addition? Scratchpad does not have this. Keep in mind scratchpad toStructuredText also does goes through the sampling function before being called however whose return value will be used for toStructuredText and is accurate.

@ecmel
Copy link
Collaborator

ecmel commented Jan 20, 2025

@acheung12 This should be fixed in Insights scratchpad as well. (https://kxl.atlassian.net/browse/KXI-58865) since evaulate.q and Insights returns the same response for the specific query.

@acheung12
Copy link
Collaborator

@acheung12 This should be fixed in Insights scratchpad as well. (https://kxl.atlassian.net/browse/KXI-58865) since evaulate.q and Insights returns the same response for the specific query.

Scratchpad handles the raw response which looks correct to me. The results view is UI?

@ecmel
Copy link
Collaborator

ecmel commented Jan 20, 2025

Insights scratchpad returns the following for structuredText:

data ='{"count":4,"columns":{"name":"values","type":"chars","values":["\\"Done\\""],"order":[0]}}'

It has only 1 value in array but count is 4. If this is correct, we should refactor extension code.

@acheung12
Copy link
Collaborator

@ecmel This is correct. Strings in Q are considered to be char vectors. Therefore, the count is "done" is 4 AKA 4 characters. I'll bring this up to someone on Magenta if you wish for a redesign but I believe this is correct.

Below is a unit test that tests for the count and tostructText feature on scratchpad. This is considered correct.
image

@kx-sonarqube
Copy link

kx-sonarqube bot commented Jan 22, 2025

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@acheung12 acheung12 merged commit 0f9ec0c into dev Jan 22, 2025
5 checks passed
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.

4 participants