Skip to content

none encding string sort error!!!!#358

Open
thzyyx wants to merge 1 commit into
heavyai:masterfrom
thzyyx:patch-1
Open

none encding string sort error!!!!#358
thzyyx wants to merge 1 commit into
heavyai:masterfrom
thzyyx:patch-1

Conversation

@thzyyx

@thzyyx thzyyx commented Jun 19, 2019

Copy link
Copy Markdown

when use kENCODING_NONE string sort, result is error,i find the promble is that system get data use ResultSet index. we should use actually line number(ival_copy).

when use kENCODING_NONE string sort, result is error,i find the promble is that system get data use ResultSet index. we should use actually line number(ival_copy).
@CLAassistant

CLAassistant commented Jun 19, 2019

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@asuhan

asuhan commented Jun 19, 2019

Copy link
Copy Markdown
Contributor

We need a test case and a clear description of the error. What is the error message?

@thzyyx

thzyyx commented Jun 19, 2019

Copy link
Copy Markdown
Author

String sort order error and table have line number must greater than 5;
step 1:
CREATE TABLE test3 (
text1 TEXT ENCODING DICT(32),
text2 TEXT ENCODING DICT(16),
text3 TEXT ENCODING DICT(8),
text4 TEXT ENCODING NONE)
step2: insert data ,data source
"kBRBwGIx","kBRBwGIx","kBRBwGIx","kBRBwGIx"
"lJRk9D07","lJRk9D07","lJRk9D07","lJRk9D07"
"g8mfeLgN","g8mfeLgN","g8mfeLgN","g8mfeLgN"
"ua0TMyzp","ua0TMyzp","ua0TMyzp","ua0TMyzp"
"EkQsLWDy","EkQsLWDy","EkQsLWDy","EkQsLWDy"
"ik5+Bf/1","ik5+Bf/1","ik5+Bf/1","ik5+Bf/1"
"CTm+quPU","CTm+quPU","CTm+quPU","CTm+quPU"
"UCyKluLc","UCyKluLc","UCyKluLc","UCyKluLc"
"S5gKOILj","S5gKOILj","S5gKOILj","S5gKOILj"
"kMhL82ga","kMhL82ga","kMhL82ga","kMhL82ga"
step3: select and sort
select rowid,text1,text2,text3,text4 from test3 order by text4;
ERROR RESULT:
4|EkQsLWDy|EkQsLWDy|EkQsLWDy|EkQsLWDy
8|S5gKOILj|S5gKOILj|S5gKOILj|S5gKOILj
6|CTm+quPU|CTm+quPU|CTm+quPU|CTm+quPU
5|ik5+Bf/1|ik5+Bf/1|ik5+Bf/1|ik5+Bf/1
2|g8mfeLgN|g8mfeLgN|g8mfeLgN|g8mfeLgN
9|kMhL82ga|kMhL82ga|kMhL82ga|kMhL82ga
0|kBRBwGIx|kBRBwGIx|kBRBwGIx|kBRBwGIx
7|UCyKluLc|UCyKluLc|UCyKluLc|UCyKluLc
1|lJRk9D07|lJRk9D07|lJRk9D07|lJRk9D07
3|ua0TMyzp|ua0TMyzp|ua0TMyzp|ua0TMyzp

@asuhan

asuhan commented Jun 21, 2019

Copy link
Copy Markdown
Contributor

@alexbaden I believe we have an internal issue about this. Does this fix look correct to you?

@thzyyx

thzyyx commented Jun 21, 2019

Copy link
Copy Markdown
Author

1、First of all, this bug exists and my method can output correct results.
2、other branch "lazy_decode(col_lazy_fetch, frag_col_buffer, ival_copy);" use actual line number( ival_copy),Why do non-encodings string need to use a result buffer index(storage_lookup_result.fixedup_entry_idx) to get data instead of real line Numbers(ival_copy),If it is a special design, please explain

@asuhan

asuhan commented Jun 21, 2019

Copy link
Copy Markdown
Contributor

I agree it returns the correct result for this particular case, but we still have to understand what is going on. The "special design" here is that we have to support tables which contain multiple fragments, your fix might or might not be correct -- we need to think it through.

@alexbaden

Copy link
Copy Markdown
Contributor

Hi @thzyyx . I ran this through our internal test suite and it passed. I need a bit more time to look at the fix, so we can understand what is going on (like @asuhan said). In the meantime, could you add a unit test to ExecuteTest.coo to reproduce the bug and ensure we have coverage for the fix? The test script will generate data and run SQL queries through the system (with the option to compare w/ sqlite for correctness). You should be able to use the existing data and just add a query to one of the test blocks -- try running ./ExecuteTest --keep-data from the build/tests directory. That will run the tests but not delete the data afterwards, so you can connect a server to the tests/tmp directory and inspect the tables.

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