Navigation Menu

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

Remove unnecessary condition in fullScan #611

Merged
merged 1 commit into from Dec 1, 2016

Conversation

brainopia
Copy link
Contributor

@brainopia brainopia commented Nov 29, 2016

There are two reasons to remove this condition.

  1. It's not used. All conditions when attribute is present are
    already coverered via index lookup in other branches of switch in getProposal
    (*a**,ea**,*av**,eav*,***n).

  2. It's broken. Default branch of switch (which corresponds to full scan) in
    getProposal always populates providing in order of EAV, but if this
    condition would ever run it would return values in order of AVE thus
    breaking the assumptions of joinRound function.


This change is Reviewable

There are two reasons to remove this condition.
1. It's not used. All conditions when `attribute` is present are
already coverered via index lookup in other branches of switch in getProposal
(*a**,ea**,*av**,eav*,***n).
2. It's broken. Default branch of switch (which corresponds to FullScan) in
getProposal always populates `providing` in order of EAV, but if this
condition would ever run it would return values in order of AVE thus
breaking the assumptions of joinRound function.
@brainopia
Copy link
Contributor Author

By the way, this PR is a by-product of a work on another PR. Currently in Eve, lookup in search section is broken unless record,attribute and value are all present in code.
Example,

  prepare
  ~~~
    commit
      [#foo]
  ~~~

  test
  ~~~
    search
      lookup[record attribute]
  ~~~

If you run it in shell you'll get

prefix[currentProvide.id] = value[providingIx];
                                     ^
TypeError: Cannot read property 'id' of undefined
    at joinRound (/home/brainopia/code/sources/eve/build/src/runtime/join.js:709:38)

The missing parts of lookup are propagated as undefined to Scan and it breaks all assumptions of current logic. I have a plan to tackle this and half-way through implementing it.

Copy link
Contributor

@ibdknox ibdknox left a comment

Choose a reason for hiding this comment

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

Originally I added this because technically you could use fullscan outside of just proposals, but there are probably better mechanisms for that. The only thing this adds is potentially a bit of a speed boost by starting with the attribute first if it's fixed, but since that doesn't happen anywhere right now, I guess we can remove it.

Thanks!

@ibdknox ibdknox merged commit 1dbbaf4 into witheve:master Dec 1, 2016
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.

None yet

2 participants