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

Fix issues with nested dicts #2253

Merged
merged 3 commits into from
Aug 7, 2023
Merged

Conversation

kabra1110
Copy link
Collaborator

Fixes issues to enable use of nested dictionaries.

@kabra1110
Copy link
Collaborator Author

Cases fixed

from lpython import i32

def test_nested_dict():
    d: dict[i32, dict[i32, i32]] = {1: {2: 3}}
    d[1] = {}       # Segmentation Fault

test_nested_dict()
from lpython import i32

def test_nested_dict():
    d: dict[i32, dict[i32, i32]] = {1: {2: 3}}
    d[1][4] = 5     # KeyError

test_nested_dict()
from lpython import i32

def test_nested_dict():
    d: dict[i32, dict[i32, i32]] = {}
    d[1] = {}
    assert len(d) == 1
    assert len(d[1]) == 0
    d[1][2] = 3     # FP Exception (!?)

test_nested_dict()
from lpython import i32

def test_nested_dict():
    d: dict[i32, dict[i32, i32]] = {1: {2: 3}}
    assert len(d) == 1
    assert len(d[1]) == 1
    d[1][3] = 4     # vs here KeyError

test_nested_dict()

Some tests

from lpython import i32

def test_nested_dict():
    d: dict[i32, dict[i32, i32]] = {1001: {2002: 3003}}
    dx: dict[i32, i32]
    d[1001][2003] = 4005
    dx = d[1001]
    print(dx.keys())

test_nested_dict()
from lpython import i32

def test_nested_dict():
    d: dict[i32, dict[i32, dict[i32, i32]]] = {1001: {2002: {3003: 4004}}}
    dx: dict[i32, i32]
    d[1001][2002][4005] = 5006
    dx = d[1001][2002]
    print(dx.keys())

test_nested_dict()
def test_nested_dict():
    d: dict[i32, dict[i32, dict[i32, list[i32]]]] = {1001: {2002: {3003: [4004]}}}
    dx: dict[i32, list[i32]]
    d[1001][2002][4005] = [4004, 5006]
    dx = d[1001][2002]
    print(dx.keys(), dx[3003], dx[4005])

test_nested_dict()

@kabra1110
Copy link
Collaborator Author

Now, issue in

from lpython import i32

def test_nested_dict():
    d: dict[i32, dict[i32, i32]] = {1001: {2002: 3003}, 1002: {1: 2}}
    d[1001] = d[1002]
    d[1001][2003] = 4005

test_nested_dict()
def test_nested_dict():
    d: dict[i32, dict[i32, i32]] = {1001: {2002: 3003}, 1002: {101: 2}}
    # d[1001] = {10: 20}
    d[1001] = d[1002]
    d[1001][100] = 4005
    # issue is only in this combination... (hangs)
    # d[1001][100] works if not d[1001] = d[1002]
    # and with d[1001] = d[1002], d[1001][101] works (key already +nt in d[1002])

test_nested_dict()

@kabra1110 kabra1110 force-pushed the nested_dict branch 2 times, most recently from 21289fe to 3da0e4f Compare August 5, 2023 07:58
@kabra1110
Copy link
Collaborator Author

kabra1110 commented Aug 5, 2023

Tried figuring out the issue. d[1001] = d[1002] invokes rehashing (due to threshold breach; initialization at the start results in both occupancy and capacity being 2), and post-rehash the destination does not actually point to d[1002] (which is expected). I think this could be dealt as a 'special' case.

This issue is confirmed by these prints

from lpython import i32

def test_nested_dict():
    d: dict[i32, dict[i32, i32]] = {1001: {2002: 3003}, 1002: {101: 2}}
    print(len(d[1001]), len(d[1002]))    # 1, 1
    d[1001] = d[1002]
    print(len(d[1001]), len(d[1002]))    # 0, 1

test_nested_dict()

@kabra1110
Copy link
Collaborator Author

kabra1110 commented Aug 6, 2023

Fixed above issue by calling rehash_all_at_once_if_needed() twice: before and after writing item.

But this is unexpectedly causing issues with bool keys.

An issue popped up with bool key hash due to the above changes. Fixed.

@kabra1110 kabra1110 changed the title [WIP] Fix issues with nested dicts Fix issues with nested dicts Aug 6, 2023
@certik certik requested a review from czgdp1807 August 7, 2023 00:57
@certik
Copy link
Contributor

certik commented Aug 7, 2023

Thanks, great work. @czgdp1807 can you please review it?

@kabra1110
Copy link
Collaborator Author

kabra1110 commented Aug 7, 2023

Currently we do not support operations like

l: list[list[i32]] = [[1, 2, 3], [4, 5, 6]]
l[1].append(7)  # semantic error: Only Name type and constant integers supported in Call

This should be done in python_ast_to_asr/handle_attribute with AST::is_a<AST::Subscript_t> but I'm not entirely sure on how to create the subsequent calls, since the symbol itself won't work.

This fix is important to ensure correct benchmarking of the changes in this PR.

Copy link
Collaborator

@czgdp1807 czgdp1807 left a comment

Choose a reason for hiding this comment

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

So far it looks good to me.

@czgdp1807
Copy link
Collaborator

This fix is important to ensure correct benchmarking of the changes in this PR.

How? This PR is related to dict and the issue you are talking about is lists.

@kabra1110
Copy link
Collaborator Author

How? This PR is related to dict and the issue you are talking about is lists.

I used lists to just illustrate that the issue is not restricted to this PR - it is for anything that uses subscripts.

For example, this analogous code does not work

d: dict[i32, dict[i32, i32]] = {1: {2: 3}, 4: {5: 6}}
d[1].pop(2)    # same issue

@kabra1110 kabra1110 marked this pull request as ready for review August 7, 2023 14:59
@czgdp1807
Copy link
Collaborator

Got it. Let’s fix it in a subsequent PR.

@czgdp1807 czgdp1807 merged commit b5a0ce9 into lcompilers:main Aug 7, 2023
9 checks passed
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.

3 participants