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

format on Sql #156

Closed

Conversation

archipelweb
Copy link
Contributor

Description

Allow to use a format parameter for sql plugin

Issues Resolved

#155

@dblock
Copy link
Member

dblock commented May 30, 2023

Please amend the commit with passing DCO (git commit -s --amend).

Is there some kind of test we can write to exercise this?

Signed-off-by: archipelweb <github@archipel-web.com>
@archipelweb
Copy link
Contributor Author

We done the commit

to validate this modification:

  • The basic query :
/*
load client with credentials
*/
$request="SELECT * FROM `my-index`";
$params=['query' => $request];
$result = $client->sql()->query($params);

the return:

"schema": [
    {
        "name": "date",
        "type": "text"
    }
]
"datarows": [
    [
        "2023-05-31 09:18:38",
    ]
],
"total": 1,
"size": 1,
"status": 200
  • the query with format overload :
/*
load client with credentials
*/
$params=['query' => $request];
$params['format']='json';
$result = $client->sql()->query($params);

the return:

{
    "took": 9,
    "timed_out": false,
    "_shards": {
        "total": 5,
        "successful": 5,
        "skipped": 0,
        "failed": 0
    },
    "hits": {
        "total": {
            "value": 1,
            "relation": "eq"
        },
        "max_score": 1,
        "hits": [
            {
                "_index": "my-index",
                "_id": "123456789",
                "_score": 1,
                "_source": {
                    "date": "2023-05-31 09:18:38"
                }
            }
        ]
    }
}

@dblock
Copy link
Member

dblock commented May 31, 2023

  • DCO is still not passing. LMK if you need help.
  • Needs a line in CHANGELOg
  • Needs a unit test for this change.

@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Merging #156 (0eb1482) into main (8b6cdbc) will not change coverage.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##               main     #156   +/-   ##
=========================================
  Coverage     24.14%   24.14%           
  Complexity     1937     1937           
=========================================
  Files           246      246           
  Lines          6863     6863           
=========================================
  Hits           1657     1657           
  Misses         5206     5206           
Impacted Files Coverage Δ
src/OpenSearch/Endpoints/Sql/Query.php 66.66% <0.00%> (ø)

@archipelweb
Copy link
Contributor Author

Sorry dblock,
We are some developers not used to the world of open source contributions...
Initially, we only just wanted to suggest this small improvement, by showing you what it could bring, and we have try to provide you some details to help you (as we can) to integrate it...
But right now, we can't do much better, sorry..
Is it not possible for you to "properly" integrate this small evolution (if you judge it relevant) into the project, by yourself ?
Thanks you.

@dblock
Copy link
Member

dblock commented May 31, 2023

I think it's a great small change! Without tests the feature can break quietly, and I am sure you wouldn't like that suddenly a future release gets cut and your added feature is broken! If you have time to finish this PR, thank you, if not let's hope someone picks it up from where you left it off. Thanks for getting it this far.

CSalih pushed a commit to CSalih/opensearch-php that referenced this pull request Jul 9, 2023
cherry-pick from @archipelweb see: opensearch-project#156

Signed-off-by: archipelweb <github@archipel-web.com>
CSalih pushed a commit to CSalih/opensearch-php that referenced this pull request Jul 9, 2023
cherry-pick from @archipelweb see: opensearch-project#156

Signed-off-by: archipelweb <github@archipel-web.com>
@CSalih
Copy link
Contributor

CSalih commented Jul 9, 2023

I was so kind and have extended it with tests in a new PR #161. This is a nice feature to have, thank you @archipelweb for beginning this feature.

CSalih pushed a commit to CSalih/opensearch-php that referenced this pull request Jul 9, 2023
cherry-pick from @archipelweb see: opensearch-project#156

Signed-off-by: archipelweb <github@archipel-web.com>
dblock pushed a commit that referenced this pull request Jul 12, 2023
…at (#161)

* feat: add test for SQL query endpoint

Signed-off-by: Salih Candir <salih@live.at>

* format on Sql

cherry-pick from @archipelweb see: #156

Signed-off-by: archipelweb <github@archipel-web.com>

* docs: add 'format' to allowed SQL query params

Signed-off-by: Salih Candir <salih@live.at>

* docs: update changelog

Signed-off-by: Salih Candir <salih@live.at>

* feat: make failed message of testFormatParamIsAllowedToSet more precise

Signed-off-by: Salih Candir <salih@live.at>

* refactor: add an line break at the end of line

Signed-off-by: Salih Candir <salih@live.at>

* docs: add example how to use query using sql

Signed-off-by: Salih Candir <salih@live.at>

---------

Signed-off-by: Salih Candir <salih@live.at>
Signed-off-by: archipelweb <github@archipel-web.com>
Co-authored-by: archipelweb <github@archipel-web.com>
@archipelweb
Copy link
Contributor Author

archipelweb commented Jul 12, 2023

I was so kind and have extended it with tests

Yeah, @CSalih that's really kind ! Tank you very much for this.
We are grateful !

@dblock
Copy link
Member

dblock commented Jul 13, 2023

Closing after #161 was merged. Thank @archipelweb and @CSalih for contributing!

@dblock dblock closed this Jul 13, 2023
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