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

(WIP) Bug rad reverse sorting #1021

Merged
merged 3 commits into from Mar 4, 2021
Merged

Conversation

alterx
Copy link
Contributor

@alterx alterx commented Nov 3, 2020

This PR aims to fix the issues with reverse when using RAD's lexical querying. I've tested the fix in the browser and it works, but, for some reason, the test is not passing (reverse is not working when running mocha)

@alterx
Copy link
Contributor Author

alterx commented Nov 3, 2020

I used this to test locally
You can uncomment: and add my changes to ./store.js to see reverse working

<script src="https://cdn.jsdelivr.net/npm/gun/gun.js"></script>
<script src="https://cdn.jsdelivr.net/npm/gun/sea.js"></script>
<script src="https://cdn.jsdelivr.net/npm/gun/lib/radix.js"></script>
<script src="https://cdn.jsdelivr.net/npm/gun/lib/radisk.js"></script>
<script src="https://cdn.jsdelivr.net/npm/gun/lib/store.js"></script>
<!--script src="./store.js"></script-->
<script src="https://cdn.jsdelivr.net/npm/gun/lib/rindexed.js"></script>
<script>
  (async () => {
    var arr = [
      'Adalard',
      'Adora',
      'Aia',
      'Albertina',
      'Alfie',
      'Allyn',
      'Amabil',
      'Ammamaria',
      'Andy',
      'Anselme',
      'Ardeen',
      'Armand',
      'Ashelman',
      'Aube',
      'Averyl',
      'Baker',
      'Barger',
      'Baten',
      'Bee',
      'Benia',
      'Bernat',
      'Bevers',
      'Bittner',
      'Bobbe',
      'Bonny',
      'Boyce',
      'Breech',
      'Brittaney',
      'Bryn',
      'Burkitt',
      'Cadmann',
      'Campagna',
      'Carlee',
      'Carver',
      'Cavallaro',
      'Chainey',
      'Chaunce',
      'Ching',
      'Cianca',
      'Claudina',
      'Clyve',
      'Colon',
      'Cooke',
      'Corrina',
      'Crawley',
      'Cullie',
      'Dacy',
      'Daniela',
      'Daryn',
      'Deedee',
      'Denie',
      'Devland',
      'Dimitri',
      'Dolphin',
      'Dorinda',
      'Dream',
      'Dunham',
      'Eachelle',
      'Edina',
      'Eisenstark',
      'Elish',
      'Elvis',
      'Eng',
      'Erland',
      'Ethan',
      'Evelyn',
      'Fairman',
      'Faus',
      'Fenner',
      'Fillander',
      'Flip',
      'Foskett',
      'Fredette',
      'Fullerton',
      'Gamali',
      'Gaspar',
      'Gemina',
      'Germana',
      'Gilberto',
      'Giuditta',
      'Goer',
      'Gotcher',
      'Greenstein',
      'Grosvenor',
      'Guthrey',
      'Haldane',
      'Hankins',
      'Harriette',
      'Hayman',
      'Heise',
      'Hepsiba',
      'Hewie',
      'Hiroshi',
      'Holtorf',
      'Howlond',
      'Hurless',
      'Ieso',
      'Ingold',
      'Isidora',
      'Jacoba',
      'Janelle',
      'Jaye',
      'Jennee',
      'Jillana',
      'Johnson',
      'Josy',
      'Justinian',
      'Kannan',
      'Kast',
      'Keeley',
      'Kennett',
      'Kho',
      'Kiran',
      'Knowles',
      'Koser',
      'Kroll',
      'LaMori',
      'Lanctot',
      'Lasky',
      'Laverna',
      'Leff',
      'Leonanie',
      'Lewert',
      'Lilybel',
      'Lissak',
      'Longerich',
      'Lou',
      'Ludeman',
      'Lyman',
      'Madai',
      'Maia',
      'Malvina',
      'Marcy',
      'Maris',
      'Martens',
      'Mathilda',
      'Maye',
      'McLain',
      'Melamie',
      'Meras',
      'Micco',
      'Millburn',
      'Mittel',
      'Montfort',
      'Moth',
      'Mutz',
      'Nananne',
      'Nazler',
      'Nesta',
      'Nicolina',
      'Noellyn',
      'Nuli',
      'Ody',
      'Olympie',
      'Orlena',
      'Other',
      'Pain',
      'Parry',
      'Paynter',
      'Pentheas',
      'Pettifer',
      'Phyllida',
      'Plath',
      'Posehn',
      'Proulx',
      'Quinlan',
      'Raimes',
      'Ras',
      'Redmer',
      'Renelle',
      'Ricard',
      'Rior',
      'Rocky',
      'Ron',
      'Rosetta',
      'Rubia',
      'Ruttger',
      'Salbu',
      'Sandy',
      'Saw',
      'Scholz',
      'Secor',
      'September',
      'Shanleigh',
      'Shenan',
      'Sholes',
      'Sig',
      'Sisely',
      'Soble',
      'Spanos',
      'Stanwinn',
      'Stevie',
      'Stu',
      'Suzanne',
      'Tacy',
      'Tanney',
      'Tekla',
      'Thackeray',
      'Thomasin',
      'Tilla',
      'Tomas',
      'Tracay',
      'Tristis',
      'Ty',
      'Urana',
      'Valdis',
      'Vasta',
      'Vezza',
      'Vitoria',
      'Wait',
      'Warring',
      'Weissmann',
      'Whetstone',
      'Williamson',
      'Wittenburg',
      'Wymore',
      'Yoho',
      'Zamir',
      'Zimmermann',
    ];

    const gun = Gun();

    let keys = JSON.parse(localStorage.getItem('testKeys'));

    if (!keys) {
      keys = await Gun.SEA.pair();
      localStorage.setItem('testKeys', JSON.stringify(keys));
      console.log('keys stored');
    }

    const insert = (node, data, i) =>
      new Promise((resolve, reject) => {
        node
          .get(data)
          .put({ name: data, age: i }, (ack) =>
            ack.err ? reject(ack.err) : resolve()
          );
      });

    gun.on('auth', async () => {
      // for (let i = 0; i < arr.length; i++) {
      //   await insert(user.get('index'), arr[i], i);
      // }
      console.log('Show items in reverse');
      user
        .get('index')
        .get({ '.': { '*': 'A' }, '%': 50000, '-': 1 })
        .map()
        .on((node, v, k) => {
          console.log(v);
        });
    });

    var user = gun.user();
    await user.auth(keys);
  })();
</script>

@amark
Copy link
Owner

amark commented Nov 9, 2020

@alterx yay for helping fix bugs! Very much appreciated.

The fact Martti can run this with Iris and no problems is a big win / indication it is correct.

Sorry I've been bogged down. When we chatted the other day it seems like mocha in the browser also had the same failure as NodeJS, so we should review that before pulling (bleeh ugh I hate saying this). I 100% predict it is bug in my code tho, not yours. My mental state hasn't been very good tho to see why (tho I'm excited jibbery that you've done this!), so is it OK if I don't pull quite yet? I feel sad I've been the bottleneck :(

@alterx
Copy link
Contributor Author

alterx commented Nov 12, 2020

@amark no problem, I need to find some time to figure out why this is not working inside the test. I definitely think we should wait until there's at least one functional unit test, this way it won't break again in the future.

One thing that's worth noting is that the previous test was being explicitly skipped, maybe an indication that this issue with testing reverse happened before?

@mimiza
Copy link
Contributor

mimiza commented Feb 4, 2021

Let's dive this PR. This thing is very important. I'm more than excited to see this epic PR get merged.

@mimiza
Copy link
Contributor

mimiza commented Feb 17, 2021

@alterx The test was unsuccessful on my browser (and neither on Node)
Screenshot from 2021-02-17 20-05-26

@mimiza
Copy link
Contributor

mimiza commented Feb 17, 2021

@alterx @amark OK I found a clue. If you run the RAD query right after put(), it won't work. But if you run the RAD query in another browser session, after put() ran and the put() command is commented out , and after refreshing your browser, IT WORKS.

It seems like the problem is somewhere within gun and this also happens to Nodejs which is the reason why the test fails.

@amark I think it is fine to merge this PR, then we could create another issue to find/fix this bug.

Screenshot from 2021-02-17 22-59-35
Screenshot from 2021-02-17 22-59-03

@amark
Copy link
Owner

amark commented Feb 17, 2021

@mimiza @alterx OK, so it means that RAD from disk is correctly returning things in order we expect, however it sounds like GUN's in-memory copy, running radix sorting is failing.

Carlos, if you 👍 also I'll go ahead an pull then, cause that means your edit is working and some other code of mine isn't. Tho either of you, please add a .skip( to the failing test and a comment for us to come back to it later. I don't red badges :P on the README. :P

@mimiza
Copy link
Contributor

mimiza commented Feb 23, 2021

@amark Carlos seems to be not online for a while. I wonder what happened to him. I don't know how to commit directly to this PR. Could you please add the skip() to the test and merge it?

@amark
Copy link
Owner

amark commented Mar 4, 2021

@mimiza @alterx pulling then will commit again with the .skip( for now.

@amark amark merged commit 45dd008 into amark:master Mar 4, 2021
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

3 participants