@fengkaijia opened this Pull Request on September 22nd 2017

#11930 only changed country and region level archive data. This PR remaps the city data so Tibetan cities are correctly displayed. Before the patch, no visits from Tibetan city is on the city level map (but visits from Tibet can be seen on the region level map).

image

Above is a screenshot on my server after applying the patch (Lhasa is now on the map).

@sgiehl commented on September 22nd 2017 Member

I'm wondering if those 8 cities are the only cities that geoip is able to detect for Tibet.

@fengkaijia commented on September 22nd 2017

@sgiehl Yes you made the right guess. I used the CSV database from http://geolite.maxmind.com/download/geoip/database/GeoLiteCity_CSV/GeoLiteCity-latest.tar.xz and run a grep, but somehow a binary character made grep stopped at the 8th record. I just tried to open the CSV in Excel and got like 50 more records. Let me figure out how to do this. Are there any getRowFromLabelEndWith alike method available?

@fengkaijia commented on September 22nd 2017

@sgiehl Do you think it's OK to add a get-the-protected-rowsIndexByLabel method to DataTable? I guess otherwise we will just do the GroupBy every time without checking the IF condition.

@sgiehl commented on September 22nd 2017 Member

Are there any getRowFromLabelEndWith alike method available?

No there isn't. Guess we should add the GroupBy filter always, without checking if it's needed. Even with having rowsIndexByLabel public we would need to iterate over all items in order to check if one ends with ti

@fengkaijia commented on September 22nd 2017

Added a new patch, again have tested to be working on my server.

I'm not sure what the GroupBy does. If it's costly then I guess it's still better to run an checking iteration. We can avoid doing the GroupBy in most cases since there are not many Internet users (or simply a large population) in Tibet (I myself run a Chinese tech blog and got less than 50 visits from Tibet in 4 years).

@sgiehl commented on September 22nd 2017 Member
@fengkaijia commented on September 23rd 2017

Well, to me, it does seem worth it to run a pre-check on rowsIndexByLabel. What do you think?

@sgiehl commented on September 26th 2017 Member

Well, to me, it does seem worth it to run a pre-check on rowsIndexByLabel. What do you think?

I agree, but we should avoid to make rowsIndexByLabel public accessible.

@fengkaijia commented on September 27th 2017

Yes, fair point. I guess then we don't need to change the PR since adding getRowFromLabelStartsWith and getRowFromLabelEndsWith to DataTable is too redundant, while a getRowFromLabelRegex also cost performance.

Powered by GitHub Issue Mirror