Page MenuHomePhabricator

mw.ext.data.get() returns 0-indexed tables
Closed, ResolvedPublic

Description

Lua's 1-based table index is working for the data returned by the mw.ext.data.get() function. The first column is returned as [0] value. Is there an easy way to recode an array of arrays to Lua's spec?

/me hates 1-based indexes :(

Event Timeline

Yurik created this task.Dec 9 2016, 10:27 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 9 2016, 10:27 PM

Look at Scribunto_LuaTextLibrary::reindexArrays() ? It's private though.

Yurik added subscribers: MaxSem, debt.EditedDec 13 2016, 8:43 AM

@Anomie, what do you think? Should we waste (considerable?) array reindexing resources to convert, in some cases nearly 2MB of array of arrays? The data is structured as [ [...], [...], [...], ...], so this would be

$rowIndexes = range( 1, $columnCount );
$result = array_combine(
  range( 1, count( $data ) ),
  array_map( function ( $row ) use ( $rowIndexes ) {
    return array_combine( $rowIndexes, $row );
  }, $data
) );

We have to urgently make this decision - if needed, this must go onto this week's train, or else we might have a very hard time to change it afterwards.

PROs: Parsing is easier with unpack(), instead of a non-obvious val1=row[0]:

for key, row in pairs(mw.ext.data.get(dataPage).data) do
  local val1, val2, val3 = unpack(row)
  ...
Yurik added a comment.Dec 13 2016, 9:23 AM

Upon further thinking, this is a bit more complex - mw.ext.data.get() works for all of Data: namespace, which means that if one gets geojson data via Lua, it will be a huge number of two element arrays, of arrays of arrays, so converting it becomes even more expensive. I am starting to think that the convenience of the unpack(row) is not worth it, or if there should be a special reindexing just for the tabular data.

Change 326906 had a related patch set uploaded (by Yurik):
Reindex tabular data array for easier lua access

https://gerrit.wikimedia.org/r/326906

@Anomie, what do you think? Should we waste (considerable?) array reindexing resources to convert, in some cases nearly 2MB of array of arrays?

Probably. If you don't, it'd be as confusing as if in JavaScript you were to get { 2: "bar", 1: "foo" } from some function call instead of [ "foo", "bar" ].

Yurik moved this task from Backlog to In progress on the Maps-Sprint board.Dec 13 2016, 6:18 PM
hashar added a subscriber: hashar.Dec 14 2016, 10:21 AM

So from a quick discussion with Yurik. We should SWAT https://gerrit.wikimedia.org/r/#/c/326906/ , it has barely any user impact see the feature is still experimental.

A test case is the https://en.wikipedia.org/wiki/User:Yurik/WeatherDemo which renders a table based on the data at https://commons.wikimedia.org/wiki/Data:Weather/New_York_City.tab . The change pushed should make the table slightly nicer.

Another view is https://en.wikipedia.org/wiki/User:Yurik/Weather_barchart

@Anomie It was too late for yurik and I to babysit the deployment right now. Lets do that on the next swat slot or whenever you can ?

don't worry if the table gets broken after the deployment - the module https://en.wikipedia.org/wiki/Module:Sandbox/Yurik has

local date = row[0]
local highTemp, avgHighTemp, avgLowTemp, lowTemp, precip, snowfall, precipDays, snowfallDays = unpack(row)

which should be changed to

local date, highTemp, avgHighTemp, avgLowTemp, lowTemp, precip, snowfall, precipDays, snowfallDays = unpack(row)

Change 327246 had a related patch set uploaded (by Yurik):
Reindex tabular data array for easier lua access

https://gerrit.wikimedia.org/r/327246

Change 327247 had a related patch set uploaded (by Yurik):
Reindex tabular data array for easier lua access

https://gerrit.wikimedia.org/r/327247

Change 326906 merged by jenkins-bot:
Reindex tabular data array for easier lua access

https://gerrit.wikimedia.org/r/326906

Change 327246 merged by jenkins-bot:
Reindex tabular data array for easier lua access

https://gerrit.wikimedia.org/r/327246

Change 327247 merged by jenkins-bot:
Reindex tabular data array for easier lua access

https://gerrit.wikimedia.org/r/327247

Mentioned in SAL (#wikimedia-operations) [2016-12-14T19:52:35Z] <thcipriani@tin> Synchronized php-1.29.0-wmf.6/extensions/JsonConfig/includes/JCLuaLibrary.php: SWAT: [[gerrit:327246|Reindex tabular data array for easier lua access]] T152809 (duration: 00m 40s)

Mentioned in SAL (#wikimedia-operations) [2016-12-14T20:11:33Z] <thcipriani@tin> Synchronized php-1.29.0-wmf.5/extensions/JsonConfig/includes/JCLuaLibrary.php: SWAT: [[gerrit:327246|Reindex tabular data array for easier lua access]] T152809 (duration: 00m 41s)

Yurik moved this task from In progress to Done on the Maps-Sprint board.Dec 14 2016, 8:13 PM
MaxSem closed this task as Resolved.May 25 2017, 2:32 PM
MaxSem claimed this task.

Yes, thanks for reminding.