@diosmosis opened this Pull Request on November 2nd 2018 Member

Currently if a segment has a custom $sqlSegment that includes functions/operations (like UNIX_TIMESTAMP(log_visit.visit_first_action_time) - log_visit.days_since_first_visit * 86400), the joins for segment expressions won't be discovered. So if we try to use this segment w/ log_conversion, it will skip the log_visit join and fail.

This PR fixes that by parsing out table/column names from the segment expression.

Also includes a new public method to return the ArchiveWriter instance in ArchiveProcessor so it's easier to create new ArchiveProcessor instances from within Archivers.

@tsteur commented on November 4th 2018 Member

Can you add some tests for this @diosmosis ?

@diosmosis commented on November 4th 2018 Member

👍 will do

@diosmosis commented on November 5th 2018 Member

Added tests.

@tsteur commented on December 9th 2018 Member

Tested and worked. Only thing I would have done be to maybe write some tests for parseColumnsFromSqlExpr($field) to make sure it works in various cases. I think the other tests don't quite cover this as extensively? not sure... maybe they do indirectly (I think so).

I used xdebug to see what the inputs are, how they are generated etc. to see about possible inputs etc... seeing maybe just a few examples in tests might help understand what is being down there but all good if not adding more tests and if it is indirectly covered by other tests.

@diosmosis commented on December 9th 2018 Member

Only thing I would have done be to maybe write some tests for parseColumnsFromSqlExpr($field) to make sure it works in various cases.

All it should do is look for table.column expressions in strings. Ie, it doesn't parse the SQL expression into pieces at all, so the only real variants are having one vs. many & backticks vs. no backticks. Ie, the presence of operators, function calls shouldn't have an effect.

@tsteur commented on December 9th 2018 Member

oki 👍

@diosmosis commented on December 10th 2018 Member

Added a test as you suggested. Will merge if the build passes.

This Pull Request was closed on December 10th 2018
Powered by GitHub Issue Mirror