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

Allow result iteration in GroupBy ResultTransformer #514

Closed
TuomasKiviaho opened this Issue Sep 30, 2013 · 10 comments

Comments

Projects
None yet
2 participants
@TuomasKiviaho
Contributor

TuomasKiviaho commented Sep 30, 2013

This improvement would allow iteration though key/value pairs.
In addition there should be so sort of count down expression that would restrict the amount of aggregated elements per .

CloseableIterator<Map<>> results = query.transform(groupBy(t.id, t.date, count(1000)).as(iterator(e.name, e.salary)));

This could be used for instance to solve situation described in https://hibernate.atlassian.net/browse/HHH-1123

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Sep 30, 2013

Contributor

The example was a modification from #473 and I noticed that I made a mistake in what would be the result. The correct form would probably be Map<List<?>,Group> and from group I'd be calling getIterator.

To me the most appealing solution would be to treat the GroupByBuilder as Projectable.

CloseableIterator<Tuple> tuples = query.transform(groupBy(t.id, t.date, count(1000))
.iterate(new QTuple(t.id, t.date, list(QPair.create(e.name, e.salary)))));
Contributor

TuomasKiviaho commented Sep 30, 2013

The example was a modification from #473 and I noticed that I made a mistake in what would be the result. The correct form would probably be Map<List<?>,Group> and from group I'd be calling getIterator.

To me the most appealing solution would be to treat the GroupByBuilder as Projectable.

CloseableIterator<Tuple> tuples = query.transform(groupBy(t.id, t.date, count(1000))
.iterate(new QTuple(t.id, t.date, list(QPair.create(e.name, e.salary)))));
@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Oct 1, 2013

Member

Querydsl groups the group by results into a Map, so lazy iteration is not really compatible with this approach.

What is the use case for this? Do you require lazy iteration of results or do you just want a shortcut for group by result iteration?

Member

timowest commented Oct 1, 2013

Querydsl groups the group by results into a Map, so lazy iteration is not really compatible with this approach.

What is the use case for this? Do you require lazy iteration of results or do you just want a shortcut for group by result iteration?

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Oct 2, 2013

Contributor

Lazy iteration is what I had in mind.

More than often there are situations where I'd like to group together values as lists but the result set is just too vast to be dumped out all-at-once. Currently I use orderBy clause and iterate the results in lexical order. Then I do some batch processing when certain values change or custom count down triggers reach zero. I'd rather be declaring an understandable group by clause and concentrate on the batch process itself.

I was thinking that if GroupCollector could publicly announce that it will not accept anymore offered values (Currently GOne only allows one time addition for instance), then this the implementation could be turned to such that new group would be formed in such situation and iteration could be let to proceed. The current approach could be achieved later on at GroupByBuilder level where a set of new methods similar to Projectable could be placed. There would still be the Map & Group interface, but along that you could utilize expressions as well (QTuple/QBean etc.) to produce more fine grained results.

Problem is of course that this doesn't quite fit to the current API design without breaking backwards compatibility.

Contributor

TuomasKiviaho commented Oct 2, 2013

Lazy iteration is what I had in mind.

More than often there are situations where I'd like to group together values as lists but the result set is just too vast to be dumped out all-at-once. Currently I use orderBy clause and iterate the results in lexical order. Then I do some batch processing when certain values change or custom count down triggers reach zero. I'd rather be declaring an understandable group by clause and concentrate on the batch process itself.

I was thinking that if GroupCollector could publicly announce that it will not accept anymore offered values (Currently GOne only allows one time addition for instance), then this the implementation could be turned to such that new group would be formed in such situation and iteration could be let to proceed. The current approach could be achieved later on at GroupByBuilder level where a set of new methods similar to Projectable could be placed. There would still be the Map & Group interface, but along that you could utilize expressions as well (QTuple/QBean etc.) to produce more fine grained results.

Problem is of course that this doesn't quite fit to the current API design without breaking backwards compatibility.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Nov 18, 2013

Member

@TuomasKiviaho Could you review if the implementation works for you?

Member

timowest commented Nov 18, 2013

@TuomasKiviaho Could you review if the implementation works for you?

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Nov 20, 2013

Contributor

Thanks a lot,

It worked out-of-the-box for me. And you nailed it with just by changing the SPI.

Only problem I had was with QList that doesn't allow nulls due to usage of ImmutableList. I had achieved the immutability though following snippet.

    public List<?> newInstance(Object... args) {
        return Collections.unmodifiableList(Arrays.asList(args));
    }
Contributor

TuomasKiviaho commented Nov 20, 2013

Thanks a lot,

It worked out-of-the-box for me. And you nailed it with just by changing the SPI.

Only problem I had was with QList that doesn't allow nulls due to usage of ImmutableList. I had achieved the immutability though following snippet.

    public List<?> newInstance(Object... args) {
        return Collections.unmodifiableList(Arrays.asList(args));
    }
@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Nov 20, 2013

Member

Could you create a new ticket for the issues with QList?

Member

timowest commented Nov 20, 2013

Could you create a new ticket for the issues with QList?

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Nov 21, 2013

Contributor

There seems to be a malfunction iteration grouping. Map2 test yields to

1: {7=comment 7, 8=comment 8}, 
2: {1=comment 1, 2=comment 2, 3=comment 3}, 
3: {5=comment 5, 6=comment 6}

but according to MAP2_RESULT

    protected static final Projectable MAP2_RESULTS = projectable(
            row(null, pair(7, "comment 7")),
            row(null,  pair(8, "comment 8")),
            row(1, pair(1, "comment 1")),
            row(1, pair(2, "comment 2")),
            row(1, pair(3, "comment 3")),
            row(2, pair(5, "comment 5")),
            row(3, pair(6, "comment 6"))
    );

the last two pairs should be grouped separately.

Contributor

TuomasKiviaho commented Nov 21, 2013

There seems to be a malfunction iteration grouping. Map2 test yields to

1: {7=comment 7, 8=comment 8}, 
2: {1=comment 1, 2=comment 2, 3=comment 3}, 
3: {5=comment 5, 6=comment 6}

but according to MAP2_RESULT

    protected static final Projectable MAP2_RESULTS = projectable(
            row(null, pair(7, "comment 7")),
            row(null,  pair(8, "comment 8")),
            row(1, pair(1, "comment 1")),
            row(1, pair(2, "comment 2")),
            row(1, pair(3, "comment 3")),
            row(2, pair(5, "comment 5")),
            row(3, pair(6, "comment 6"))
    );

the last two pairs should be grouped separately.

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Nov 21, 2013

Contributor

Here's a version of the test that brings out the malfunction.

    @Test
    public void Map2() {
        CloseableIterator<Map<Integer, String>> results = MAP2_RESULTS.transform(
            groupBy(postId).iterate(map(commentId, commentText)));
        List<Map<Integer, String>> actual = IteratorAdapter.asList(results);

        Object commentId = null;
        Map<Integer, String> comments = null;
        List<Map<Integer, String>> expected = new LinkedList<Map<Integer, String>>();
        for (Iterator<Tuple> iterator = MAP2_RESULTS.iterate(); iterator.hasNext();) {
            Tuple tuple = iterator.next();
            Object[] array = tuple.toArray();

            if (comments == null || !(commentId == array[0] || commentId != null && commentId.equals(array[0]))) {
                comments = new LinkedHashMap<Integer,String>();
                expected.add(comments);
            }
            commentId = array[0];
            @SuppressWarnings("unchecked")
            Pair<Integer, String> pair = (Pair<Integer, String>) array[1];

            comments.put(pair.getFirst(), pair.getSecond());
        }      
        assertEquals(expected.toString(), actual.toString());
    }
Contributor

TuomasKiviaho commented Nov 21, 2013

Here's a version of the test that brings out the malfunction.

    @Test
    public void Map2() {
        CloseableIterator<Map<Integer, String>> results = MAP2_RESULTS.transform(
            groupBy(postId).iterate(map(commentId, commentText)));
        List<Map<Integer, String>> actual = IteratorAdapter.asList(results);

        Object commentId = null;
        Map<Integer, String> comments = null;
        List<Map<Integer, String>> expected = new LinkedList<Map<Integer, String>>();
        for (Iterator<Tuple> iterator = MAP2_RESULTS.iterate(); iterator.hasNext();) {
            Tuple tuple = iterator.next();
            Object[] array = tuple.toArray();

            if (comments == null || !(commentId == array[0] || commentId != null && commentId.equals(array[0]))) {
                comments = new LinkedHashMap<Integer,String>();
                expected.add(comments);
            }
            commentId = array[0];
            @SuppressWarnings("unchecked")
            Pair<Integer, String> pair = (Pair<Integer, String>) array[1];

            comments.put(pair.getFirst(), pair.getSecond());
        }      
        assertEquals(expected.toString(), actual.toString());
    }

timowest added a commit that referenced this issue Nov 21, 2013

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Nov 21, 2013

Member

Should work now. Thanks for testing.

Member

timowest commented Nov 21, 2013

Should work now. Thanks for testing.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Dec 12, 2013

Member

Released in 3.3.0.BETA2

Member

timowest commented Dec 12, 2013

Released in 3.3.0.BETA2

@timowest timowest closed this Dec 12, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment