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

Changes to handle the double precision errors #532

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kedarbcs16
Copy link
Contributor

@kedarbcs16 kedarbcs16 commented Jan 15, 2019

Currently, there are some data verification failures for near equal values, this PR is to ignore them as data verification failures and to mark as a new type data precision failure if they are failing for the precision value, also it has changed the precision value to a more accepted value and the following changes as well:-

  1. parameterize the precision value(FEPS ksay)
    2)checking for 2 more precision values for the same value in case of an error (i.e. FEPS * 10 AND FEPS * 100)

The results previously were for eg:-
Query: /root/drillAutomation/master/framework/resources/Advanced/tpch/tpch_sf1/sanity/maprdb/json/query10_sum.sql
select sum(L_DISCOUNT) from lineitem

Baseline: /root/drillAutomation/master/framework/resources/Advanced/tpch/tpch_sf1/sanity/maprdb/json/query10_sum.e_tsv
Expected number of rows: 1
Actual number of rows from Drill: 1
Number of matching rows: 0
Number of rows missing: 1
Number of rows unexpected: 1

These rows are not expected (first 10):
300057.3300002678

These rows are missing (first 10):
300057.3299997481 (1 occurence(s)

After Proposed Change:-
[PASS] (10.51 s) /root/drillAutomation/master/framework/resources/Advanced/tpch/tpch_sf1/sanity/maprdb/json/query10_sum.sql

There is only one failure in data verification remaining which looks like an actual failure:-
These rows are not expected (first 10):
able AAAAAAAAABBAAAAA 5201 1 1 1 1 1 1 1
able AAAAAAAAABBAAAAA 5202 1 1 1 1 1 1 1
able AAAAAAAAABBAAAAA 5203 1 1 1 1 1 1 1

These rows are missing (first 10):
able AAAAAAAAABBAAAAA 5201 1.02596238783322112347 0.92348065982403962962 1.03775175070487385991 1.1958496976011165 0.89227580926364466986 0.93194781639240754520 1.4127765679517665 (24 occurence(s))
able AAAAAAAAABBAAAAA 5202 1.1275577509523670 0.85947464754000782300 0.68491126287602118020 0.96069716471214886353 1.00142901006058381877 1.10846217231602910574 1.1287102632635932 (49 occurence(s))
able AAAAAAAAABBAAAAA 5203 1.01586468414891918392 0.95815542511844994124 0.93023648307049253221 0.95005761509395845600 0.83205349185674132545 0.94734936141143506888 0.93014764945033420991 (27 occurence(s))

@Agirish
Copy link
Member

Agirish commented Jan 16, 2019

Hey Kedar,

Can we sync up in person tomorrow or the day after to go over this? It'll be easier. Plus I want to run through multiple tests - as this is updating core logic.

@kedarbcs16
Copy link
Contributor Author

Hey Kedar,

Can we sync up in person tomorrow or the day after to go over this? It'll be easier. Plus I want to run through multiple tests - as this is updating core logic.

ok

@agozhiy
Copy link
Contributor

agozhiy commented Jan 23, 2019

@kedarbcs16, @Agirish, what is the status of this?

@Agirish
Copy link
Member

Agirish commented Jan 23, 2019

Had a discussion with Kedar - we will treat precision issues as a separate category from regular data verification failures. And he'll use the queries here to test the new logic. @kedarbcs16 please share an update once you have made progress.

@kkhatua
Copy link
Contributor

kkhatua commented Feb 20, 2019

@kedarbcs16 (cc: @Agirish )
what is the status of the precision check ? @agozhiy also had PR #527 that does the same. We should close one if the other is ready to be merged.

@Agirish Agirish changed the title Changes to handle the decimal precision errors Changes to handle the double precision errors Mar 8, 2019
@arina-ielchiieva
Copy link

@kkhatua / @Agirish do you know the status of this PR? Will it be merged?

@Agirish
Copy link
Member

Agirish commented Apr 1, 2019

@arina-ielchiieva , I this this PR needs some rework. We do not have the bandwidth to look into it anytime soon. So I think we will get Anton's PR merged in.

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.

5 participants