Skip to content

Commit

Permalink
Fixed CORE-5974 - Wrong result of select distinct with decfload/timez…
Browse files Browse the repository at this point in the history 
…one/collated column.

Related to e3b0247.
  • Loading branch information
@asfernandes
asfernandes committed Dec 19, 2018
1 parent ac2f012 commit 8258e51
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 20 deletions.
39 changes: 20 additions & 19 deletions src/jrd/sort.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ void Sort::get(thread_db* tdbb, ULONG** record_address)

if (record)
{
diddleKey((UCHAR*) record->sort_record_key, false);
diddleKey((UCHAR*) record->sort_record_key, false, false);
}
}
catch (const BadAlloc&)
Expand Down Expand Up @@ -379,7 +379,7 @@ void Sort::put(thread_db* tdbb, ULONG** record_address)

if (record != (SR*) m_end_memory)
{
diddleKey((UCHAR*) (record->sr_sort_record.sort_record_key), true);
diddleKey((UCHAR*) (record->sr_sort_record.sort_record_key), true, false);
}

// If there isn't room for the record, sort and write the run.
Expand Down Expand Up @@ -450,7 +450,7 @@ void Sort::sort(thread_db* tdbb)
{
if (m_last_record != (SR*) m_end_memory)
{
diddleKey((UCHAR*) KEYOF(m_last_record), true);
diddleKey((UCHAR*) KEYOF(m_last_record), true, false);
}

// If there aren't any runs, things fit nicely in memory. Just sort the mess
Expand Down Expand Up @@ -698,7 +698,7 @@ void Sort::releaseBuffer()


#ifdef WORDS_BIGENDIAN
void Sort::diddleKey(UCHAR* record, bool direction)
void Sort::diddleKey(UCHAR* record, bool direction, bool duplicateHandling)
{
/**************************************
*
Expand Down Expand Up @@ -818,7 +818,7 @@ void Sort::diddleKey(UCHAR* record, bool direction)
break;

case SKD_dec64:
if (direction)
if (direction && !duplicateHandling)
{
((Decimal64*) p)->makeKey(lwp);
*p ^= 1 << 7;
Expand All @@ -842,7 +842,7 @@ void Sort::diddleKey(UCHAR* record, bool direction)

case SKD_dec128:
fb_assert(false); // diddleKey for Dec64/128 not tested on bigendians!
if (direction)
if (direction && !duplicateHandling)
{
((Decimal128*) p)->makeKey(lwp);
*p ^= 1 << 7;
Expand Down Expand Up @@ -897,7 +897,7 @@ void Sort::diddleKey(UCHAR* record, bool direction)


#else
void Sort::diddleKey(UCHAR* record, bool direction)
void Sort::diddleKey(UCHAR* record, bool direction, bool duplicateHandling)

This comment has been minimized.

Sign in to view

Copy link
@aafemt

aafemt Dec 26, 2018

Contributor

This new parameter is not needed. In cases when exact sort order is not important (as for duplicates handling) you must set "direction" to false and that's all.
{
/**************************************
*
Expand Down Expand Up @@ -1005,7 +1005,7 @@ void Sort::diddleKey(UCHAR* record, bool direction)
}
}

if (direction || !(key->skd_flags & SKD_separate_data))
if ((direction && !duplicateHandling) || !(key->skd_flags & SKD_separate_data))
{
longs = n >> SHIFTLONG;
while (--longs >= 0)
Expand Down Expand Up @@ -1114,7 +1114,7 @@ void Sort::diddleKey(UCHAR* record, bool direction)
#endif // IEEE

case SKD_dec64:
if (direction)
if (direction && !duplicateHandling)
{
((Decimal64*) p)->makeKey(lwp);
p[3] ^= 1 << 7;
Expand All @@ -1137,7 +1137,7 @@ void Sort::diddleKey(UCHAR* record, bool direction)
break;

This comment has been minimized.

Sign in to view

Copy link
@aafemt

aafemt Dec 20, 2018

Contributor

Shouldn't be here another "else" for cases when both conditions above failed? It won't be pleasant to get an empty key on definite combination of flags.

This comment has been minimized.

Sign in to view

Copy link
@asfernandes

asfernandes Dec 21, 2018

Author Member

An "else" with what? The idea of e3b0247 is to just not run unneeded code.

This comment has been minimized.

Sign in to view

Copy link
@aafemt

aafemt Dec 21, 2018

Contributor

What worries me is that this routine become more and more spagetty code controlled by flags. And the flag you have added may lead execution flow directly into fb_assert(false). Without further comments it is hard to say why assert is there and which bad things can happen because of that.

case SKD_dec128:
if (direction)
if (direction && !duplicateHandling)
{
((Decimal128*) p)->makeKey(lwp);
p[3] ^= 1 << 7;
Expand Down Expand Up @@ -1320,19 +1320,20 @@ sort_record* Sort::getMerge(merge_control* merge)

if (l == 0 && m_dup_callback)
{
diddleKey((UCHAR*) merge->mrg_record_a, false);
diddleKey((UCHAR*) merge->mrg_record_b, false);
diddleKey((UCHAR*) merge->mrg_record_a, false, true);
diddleKey((UCHAR*) merge->mrg_record_b, false, true);

if ((*m_dup_callback) ((const UCHAR*) merge->mrg_record_a,
(const UCHAR*) merge->mrg_record_b,
m_dup_callback_arg))
{
merge->mrg_record_a = NULL;
diddleKey((UCHAR*) merge->mrg_record_b, true);
diddleKey((UCHAR*) merge->mrg_record_b, true, true);
continue;
}
diddleKey((UCHAR*) merge->mrg_record_a, true);
diddleKey((UCHAR*) merge->mrg_record_b, true);

diddleKey((UCHAR*) merge->mrg_record_a, true, true);
diddleKey((UCHAR*) merge->mrg_record_b, true, true);
}

if (l == 0)
Expand Down Expand Up @@ -2103,8 +2104,8 @@ void Sort::sortBuffer(thread_db* tdbb)
DO_32_COMPARE(p, q, l);
if (l == 0)
{
diddleKey((UCHAR*) *i, false);
diddleKey((UCHAR*) *j, false);
diddleKey((UCHAR*) *i, false, true);
diddleKey((UCHAR*) *j, false, true);

if ((*m_dup_callback) ((const UCHAR*) *i, (const UCHAR*) *j, m_dup_callback_arg))
{
Expand All @@ -2113,10 +2114,10 @@ void Sort::sortBuffer(thread_db* tdbb)
}
else
{
diddleKey((UCHAR*) *i, true);
diddleKey((UCHAR*) *i, true, true);
}

diddleKey((UCHAR*) *j, true);
diddleKey((UCHAR*) *j, true, true);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/jrd/sort.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ class Sort
void allocateBuffer(MemoryPool&);
void releaseBuffer();

void diddleKey(UCHAR*, bool);
void diddleKey(UCHAR*, bool, bool);
sort_record* getMerge(merge_control*);
ULONG allocate(ULONG, ULONG, bool);
void init();
Expand Down

0 comments on commit 8258e51

Please sign in to comment.